-
Notifications
You must be signed in to change notification settings - Fork 277
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
BUG: Update GIZMO frontend to handle newer GIZMO versions #4470
Conversation
Hi @mtryan83, this is most welcome! I'll try to have a look at this in the next few days. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this !
I agree that this work qualifies as a fix and could in principle be included in a bugfix release, but it needs to retain backward compatibility with previous versions of GIZMO. I see you attempted to satisfy this requirement already, but failing tests seem to indicate that it's not working yet.
Anyway, I know it's still in draft mode, but here are some preliminary remarks and questions.
So looking at the some of the failing tests (the non gizmo tests), it looks like the problem is files that were previously only being considered an OWLS dataset or an EAGLE dataset are now also being considered GIZMO datasets. I don't know enough about OWLS and EAGLE formats to know if that's correct or not. Is that a test problem or I'm not distinguishing GIZMO files well enough? For the gizmo tests, like Lastly, regarding most of your comments, the _parse_parameter_file function was copied directly from |
agreed to consider it out of scope, but good to know ! |
802cbc6
to
ddac4e5
Compare
note that image test failures are unrelated (#4540), and should pass next time this branch is updated |
91447d5
to
b9ab214
Compare
The macos test failure doesn't look like it's real. Also, since I have once again been reminded why tests are important, how should I go about writing one for this case? I think it can just be a few unit tests, but obviously none of the sample datasets would trigger this. |
Indeed, it's just flaky. Rerunning fixed it. |
As for testing, I would advise adding a new sample dataset if you can manage. |
0ec11f7
to
487ac68
Compare
17d977a
to
d967d83
Compare
I agree that it's out of scope, but given the significant amount of code duplication here in |
I'm working on writing a test now, using a single snapshot from the gizmo Zeldovich pancake test problem (~20 MB). For the |
20Mb is small enough that we can afford more than one file, especially for such a critical part of the logic. I say go for it. |
snapshot_076_no_gizver.tar.gz For reference, the files can be generated using the gizmo zeldovich pancake test problem with the additional config flags COOLING and METAL. The _no_gizver then has the GIZMO_version header field manually removed. Snapshot 076 was picked randomly. |
95e0c29
to
3b358ef
Compare
Just FYI, as of right now the only "failing" test is the new test I've developed, which is failing due to the missing files. What do I need to do to get them added as test datasets @neutrinoceros ? |
Sorry I meant to reply earlier and forgot. There's a whole process to get the files permanently stored on our server, and it starts with a PR to the yt-project/website repository. I advise you take a look at previous PRs there. I'll have more time to provide more details tomorrow if need be ! |
I just put in a PR for the website. Is this the right process? |
So far, so good 😊 |
alright, I see John took care of your companion PR. Now what we need is to do in this PR is to update |
dmetal check - gizmo runs with METALS enabled but all of COOL_METAL_LINES_BY_SPECIES, GALSF_FB_FIRE_RPROCESS, GALSF_FB_FIRE_AGE_TRACERS, and STARFORGE_FEEDBACK_TRACERS disabled will have a metallicity field (aka dmetal) a length 1. This change accounts for this possibility Overriding Gadget _parse_parameter_file - newer versions of GIZMO have changed some of the header fields used by yt for determining the cosmology of the simulation. Moving this function allows for improved cosmology detection. Now check if ComovingIntegrationOn flag exists - if so, use that to determine if cosmological run and pull cosmological parameters using updated names. Otherwise, fall back to checking for OmegaLambda and setting cosmological flag from that.
I somehow deleted part of a line before the previous commit. This has been fixed.
Updated comments to point out when the switch occurred.
Co-authored-by: Clément Robert <[email protected]>
Co-authored-by: Clément Robert <[email protected]>
It looks like gizmo doesn't export any additional groups besides the Header, unlike Eagle/OWLS. So this should resolve those ambiguities.
Newer (post Apr 2021) versions of gizmo export GIZMO_version=year_of_last_commit as a header variable. We should use this in preference to checking the gas metallicity, which might not exist. Since the names of the omegas have changed, we don't want to fall back to the gadget loader in the comoving case (in the non-cosmo case it shouldn't matter?)
New test requires test files gizmo_zeldovich/snapshot_076_XX_gizver.hdf5 where XX is either wi or no. The files can be generated using the gizmo zeldovich pancake test problem with the additional config flags COOLING and METAL. The _no_gizver then has the GIZMO_version header field manually removed. Snapshot 076 was picked randomly.
3b358ef
to
da25c87
Compare
import yt; ds = yt.load_sample("gizmo_zeldovich") appears to be working (well done !). Now waiting to see if all tests pass |
I do not know why the test is still failing with |
@yt-fido test this please. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM !
@jzuhone, take your time if you still want to review this, otherwise feel free to merge !
I'd like to suggest two changes to the GIZMO frontend to fix what I believe is a potential bug and to handle updates to the hdf5 interface (this was mentioned on the Slack #help channel in December(?) but I no longer have access to that post and can't reference it). I'm starting with a draft because a) technically these are two separate issues and could go into two separate PR's, but the first is so minor it seemed unnecessary and b) I'm not sure I've resolved the 2nd issue in the proper yt fashion.
dmetal check - GIZMO runs with METALS flag enabled but without COOL_METAL_LINES_BY_SPECIES, GALSF_FB_FIRE_RPROCESS, GALSF_FB_FIRE_AGE_TRACERS, or STARFORGE_FEEDBACK_TRACERS enabled will have a per-particle metallicity field (loaded as dmetal) with shape
(NUM_PARTICLES, )
. Thus, thefh[dmetal].shape[1]
call will fail with an IndexError. I've added a check to ensurefh[dmetal]
is 2d first (fh[dmetal].ndim>1
).Overriding Gadget _parse_parameter_file - newer versions of GIZMO have changed some of the header fields (described uner Reading Snapshots in the GIZMO doc) used by yt for determining the cosmology of the simulation. I've copied this function from the Gadget/data_structures.py with modifications. Now it checks if the ComovingIntegrationOn flag exists - if so, use that to determine if this is a cosmological run and pull cosmological parameters using updated names. Otherwise, fall back to checking for OmegaLambda and setting cosmological flag from that. Copying the whole function seems overkill though, is there a better way to do this?
Creating tests for both bugs should be straightforward (I think they'd be similar to the
test_gizmo_64
test), but it doesn't look like the sample datasets are made with the newer GIZMO versions and I'm not sure how to provide any. I'll figure out the tests if that can get resolved.PR Summary
PR Checklist