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.

Sign up for GitHub

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

Jump to bottom

meshesPath & particlesPath: optional #32

Merged
merged 3 commits into from
Jan 31, 2018
Merged

meshesPath & particlesPath: optional #32

merged 3 commits into from
Jan 31, 2018

Conversation

ax3l
Copy link
Member

@ax3l ax3l commented Jan 30, 2018 •
edited
Loading

Implements that meshesPath and particlesPath are optional in openPMD 1.1.0+.

But if they are set, the directories those attributes point to must exist.

  • needs real-life testing on files with/without meshes/particles

See: openPMD/openPMD-standard#171

@ax3l ax3l requested a review from RemiLehe January 30, 2018 14:43
@ax3l ax3l mentioned this pull request Jan 30, 2018
Merged
4 tasks
@ax3l ax3l force-pushed the topic-optionalMeshesParticlesPath branch from 4d0c6cb to 3d1e69c Compare January 30, 2018 14:52
@ax3l
meshesPath & particlesPath: optional
1d38ddd 
Implements that `meshesPath` and `particlesPath` are optional
in openPMD 1.1.0+.
But if they are set, the directories those attributes point to must
exist.
@ax3l ax3l force-pushed the topic-optionalMeshesParticlesPath branch from 3d1e69c to 1d38ddd Compare January 30, 2018 15:03
@RemiLehe
Copy link
Member

I checked with the thetaMode dataset in openPMD examples, and it seems to give the correct result:

openPMD_check_h5 -i data00000400.h5 
Warning: Attribute author (recommended) does NOT exist in `/`!
Found 1 iteration(s)
Iteration 400 : found 4 meshes
Error: `basePath`+`particlesPath` are set but path 'b'/data/400/particles/'' does not exist in file!
Result: 1 Errors and 1 Warnings.

@ax3l
Copy link
Member Author

ax3l commented Jan 30, 2018 •
edited
Loading

Cool! And if you remove the particlesPath attribute from / - does this silence the validator and report "found 0 particles"? :)

RemiLehe
RemiLehe reviewed Jan 30, 2018
View reviewed changes
openpmd_validator/check_h5.py Outdated
if not valid and v:
print("`particlesPath` attribute is missing in '/' "
"(will not search for particle records)")
particles_path = None
Copy link
Member

Choose a reason for hiding this comment

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

I think that this should be:

if not valid and v:
    particles_path = None
    if v:
        print("`particlesPath` attribute is missing in '/' "
              "(will not search for particle records)")
else:
    particles_path = particles_path.decode()

(Same thing with meshes_path)

Otherwise this raises an error when trying to decode None.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh yes!

@RemiLehe
Copy link
Member

Nope. It raises an error (see inline comments...)

@ax3l
Copy link
Member Author

ax3l commented Jan 30, 2018

Thanks, pushed the fix!

@ax3l ax3l force-pushed the topic-optionalMeshesParticlesPath branch from ea378dd to a59f8da Compare January 30, 2018 18:24
@RemiLehe
Copy link
Member

Thanks! This now works fine with the missing particlePath

openPMD_check_h5 -i example-thetaMode/hdf5/data00000400.h5 
Warning: Attribute author (recommended) does NOT exist in `/`!
Found 1 iteration(s)
Iteration 400 : found 4 meshes
Iteration 400 : found 0 particle species
Result: 0 Errors and 1 Warnings.

RemiLehe
RemiLehe requested changes Jan 30, 2018
View reviewed changes
openpmd_validator/check_h5.py Outdated
try:
list_meshes = list(f[full_meshes_path].keys())
except KeyError:
list_meshes = []
Copy link
Member

Choose a reason for hiding this comment

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

Could you remove the try / except KeyError here? (Because you are basically testing for it above, with an if / return statement)?

Copy link
Member Author

Choose a reason for hiding this comment

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

True, we are testing it properly now! :)

openpmd_validator/check_h5.py
@@ -702,20 +715,39 @@ def check_particles(f, iteration, v, extensionStates) :
result_array = np.array([ 0, 0])

# Find the path to the data
base_path = ("/data/%s/" % iteration).encode('ascii')
base_path = "/data/%s/" % iteration
Copy link
Member

Choose a reason for hiding this comment

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

Why is the previously-defined base_path (used for the meshes) not kept here?

Copy link
Member Author

Choose a reason for hiding this comment

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

it was inconsistent between meshesPath and particlesPath.
I unified both by just picking one.

openpmd_validator/check_h5.py Outdated
try:
list_species = list(f[full_particle_path].keys())
except KeyError:
list_species = []
Copy link
Member

Choose a reason for hiding this comment

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

Same remark on try / except KeyError.

@ax3l
Remove unnecessary try-except
4f09669 
With the paths now properly checked, we don't need a try
block anymore.
@ax3l
Copy link
Member Author

ax3l commented Jan 31, 2018

@RemiLehe thx for the review, I applied your suggested changes and explained the open question

@RemiLehe RemiLehe merged commit 5d14e30 into 1.1.X Jan 31, 2018
@ax3l ax3l deleted the topic-optionalMeshesParticlesPath branch January 31, 2018 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RemiLehe RemiLehe RemiLehe approved these changes

Assignees

@RemiLehe RemiLehe

Labels
format: h5 validator
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ax3l @RemiLehe