Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #438, atomic pdi_deactivate.h, allows disabling PDI's operation while maintaining valid syntax #504

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

JAuriac
Copy link

@JAuriac JAuriac commented Nov 28, 2024

Fix #438, atomic pdi_deactivate.h. Add a new header file which allows disabling PDI's operation, while maintaining valid syntax, and not modifying existing code.

List of things to check before making a PR

Before merging your code, please check the following:

  • you have added a line describing your changes to the Changelog;
    Modify pdi/CHANGELOG.md.
  • you have added unit tests for any new or improved feature;
    Added pdi/tests/test_06.
  • In case you updated dependencies, you have checked pdi/docs/CheckList.md.
    No dependencies change.
  • you have checked your code format:
    • you have checked that you respect all conventions specified in CONTRIBUTING.md;
      Atomic as feature.
    • you have checked that the indentation and formatting conforms to the .clang-format;
    • you have documented with doxygen any new or changed function / class;
      Not applicable.
  • you have correctly updated the copyright headers:
    • your institution is in the copyright header of every file you (substantially) modified;
    • you have checked that the end-year of the copyright there is the current one;
      Currently 2015-2021 on pdi.h, changed to 2024 on pdi_deactivation.h only
  • you have updated the AUTHORS file:
    • you have added yourself to the AUTHORS file;
      Julian Auriac as maintainer.
    • if this is a new contribution, you have added it to the AUTHORS file;
  • you have added everything to the user documentation:
    • any new CMake configuration option;
    • any change in the yaml config;
    • any change to the public or plugin API;
    • any other new or changed user-facing feature;
      Modified pdi/pdi/docs/Using_PDI.md.
    • any change to the dependencies;
  • you have correctly linked your MR to one or more issues:

@JAuriac JAuriac marked this pull request as ready for review December 2, 2024 15:36

#define PDI_H_

#include <paraconf.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we remove this paraconf dependency?

global_size[0] = longval;
PC_int(PC_get(conf, ".global_size.width"), &longval);
global_size[1] = longval;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you put to test all PDI functions?

global_size: { height: 60, width: 12 }

pdi:
plugins:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest you put hdf5 plugins here, to follow the case where PDI was enabled and did output. Also we can be sure from the test that the plugin is not triggered in the no-pdi case.

@@ -199,6 +199,7 @@ install(TARGETS PDI_C EXPORT PDI_C_EXPORT
)
install(FILES
"${PDI_SOURCE_DIR}/include/pdi.h"
"${PDI_SOURCE_DIR}/include/pdi_deactivation.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as we discussed, put the file under no-pdi/include/pdi.h

Copy link
Member

@benoitmartin88 benoitmartin88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be best for the end user if deactivating PDI requires the least changes.
With that idea in mind, the deactivated PDI header should be named pdi.h.
Also, another CMake should probably be included.

@jbigot
Copy link
Member

jbigot commented Dec 12, 2024

100% agreed, I think the use-case is for the user to copy this file in their own repository so that they don't require an actual installation of PDI anymore. On the CMake side, we have to find a solution that works for this use-case too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a mechanism to deactivate PDI on demand
4 participants