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

Scientific default values #1439

Open
franzpoeschel opened this issue May 10, 2023 · 1 comment
Open

Scientific default values #1439

franzpoeschel opened this issue May 10, 2023 · 1 comment

Comments

@franzpoeschel
Copy link
Contributor

franzpoeschel commented May 10, 2023

The implementation of scientific default attribute values (i.e. those prescribed as mandatory by the openPMD-standard) is currently relatively ad-hoc, and while it works ok so far, it is neither really flexible nor extensible.

Current situation: Default attributes are set in the constructors of objects. They are set to values that often don't really make sense (e.g., for a 3D grid, setting the axisLabels to {"x"} by default makes no sense). Having these defaults set in the constructor makes the class inflexible to reuse and causes issues in the backends because of duplicate attribute definitions (first the default value, then the user-specified one).

Suggested redesign:

  1. Separate logic that specifies default values from object model. I imagine that we add a separate logic that collects the standard-defined attribute logic in one central place. This could be an abstract class, implemented by multiple derived classes for multiple versions of the standard. The central methods would be something like virtual readDefaults(Attributable &) = 0; and virtual writeDefaults(Attributable &) = 0;.
  2. Instead of at construction time, defaults would be written at the time of Series::close() and Iteration::close() (not destructors, that causes too many issues down the road). This would postpone this logic to be as late as possible. The logic could take into consideration the dimensionality of meshes, that are then defined.
  3. Reading defaults would happen at the same time as now (during parsing). It would deal leniently with missing / wrong values, e.g. by just setting the default, or ignoring it.
  4. The implementation would no longer be based (exclusively) on the class type, but rather on the value of Attributable::myPath(). This increases reusability of classes.

To avoid merge conflicts, this should start after #1154 has been merged, or at least start based on that branch.

@franzpoeschel
Copy link
Contributor Author

Observation: When using #1432 to interact with data that was not created as openPMD data, adding an openPMD view is currently possible, but somewhat annoying, because one has to work around the parsing procedures that expect correct definitions. Addressing this issue should ideally also fix that by adding more leniency toward missing attribute definitions (at Record level as well as at root level). A dedicated test should cover this use case.

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

No branches or pull requests

2 participants