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

meshesPath & particlesPath: optional #171

Merged
merged 2 commits into from
Jan 25, 2018

Conversation

ax3l
Copy link
Member

@ax3l ax3l commented Jan 23, 2018

Makes the base attributes meshesPath and particlesPath optional if no meshes or no particles shall be stored. Specifies that the path they describe MUST exist as soon as they are set. If they are unset, this MUST be interpreted as "no respective record".

Implements issue: #143 (cc @mccoys)

Description

See comment in the original issue:

I propose to introduce the following relaxation and of the two attributes meshesPath and particlesPath:

  • relax the two attributes from required to optional
  • in the case an attributes is not set, handle the file as if there are no meshes (particles)
  • define that the path they describe MUST exist if the attributes are set (as most implemented it already like this in 1.0.0 and our validator does this also in a two-liner)

Affected Components

  • base

Logic Changes

if set, same parser logic as before, but it is now required that the path in the attributes is valid (does exist as a group).

If unset, interpret as "no particle/mesh" records found, respectively.

Writer Changes

No effect for before-valid files.

"Work-arounds" for particle-only/mesh-only openPMD files must now make sure that the path in meshesPath/ or particlesPath/ does really exist, since that behavior was unspecified so far. If not needed, they are now allowed to skip an attribute.

Reader Changes

Data Converter

No effect, old files are forward compatible to this change.

@ax3l ax3l added the minor change backwards-compatible change label Jan 23, 2018
@ax3l ax3l requested a review from C0nsultant January 23, 2018 13:01
@RemiLehe
Copy link
Member

Great change. I'll implement it in the openPMD-viewer.

@ax3l ax3l changed the title meshesPath & particlesPath: Optional [WIP] meshesPath & particlesPath: Optional Jan 23, 2018
@ax3l ax3l changed the title [WIP] meshesPath & particlesPath: Optional [WIP] meshesPath & particlesPath: optional Jan 23, 2018
@ax3l ax3l changed the title [WIP] meshesPath & particlesPath: optional meshesPath & particlesPath: optional Jan 23, 2018
Copy link
Collaborator

@DavidSagan DavidSagan left a comment

Choose a reason for hiding this comment

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

Looks good except "directory" should be "group"

Makes the base attributes `meshesPath` and `particlesPath` optional
if no meshes or no particles shall be stored. Specifies that the
path they describe MUST exist as soon as they are set. If they
are unset, this MUST be interpreted as "no respective record".
@ax3l
Copy link
Member Author

ax3l commented Jan 25, 2018

@DavidSagan thx for the review, fixed!

@RemiLehe
Copy link
Member

Should we merge this now?

@ax3l
Copy link
Member Author

ax3l commented Jan 25, 2018

all right! :)

@ax3l ax3l merged commit ff8686a into openPMD:upcoming-1.1.0 Jan 25, 2018
@ax3l ax3l deleted the topic-onlyMeshOrPart branch January 25, 2018 14:18
@ax3l ax3l mentioned this pull request Feb 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor change backwards-compatible change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants