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

BUGFIX: Enzo testing framework - ignore units for ShockTube tests #4764

Merged
merged 2 commits into from
Dec 21, 2023

Conversation

clairekope
Copy link
Contributor

PR Summary

This bug was found while fixing the Enzo CI testing. It seems to have been induced by the upgrade to unyt v3. The bug is as follows:

The ShockTubeTest class supports comparing simulation results to an exact solution saved to file; however, the solution in that file doesn't have any units associated with it. When trying to compare solutions, unyt complains.

This is a very simple change that removes units from the simulation data before comparing.
The test for it is essentially the Enzo test infrastructure over at https://github.com/enzo-project/enzo-dev? So I'm not sure how I would add a test for this within yt.

chrishavlin
chrishavlin previously approved these changes Dec 21, 2023
Copy link
Contributor

@chrishavlin chrishavlin 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 to me. was missed in yt's unyt 3.0 compatibility testing because yt doesn't actually have any tests that use this ShockTubeTest class. IMO, fine to merge without adding a new test, but I'll wait a bit before merging to see if anyone has any other thoughts on the matter.

@neutrinoceros
Copy link
Member

If this test code isn't used in yt itself we probably ought to remove it at some point, so if Enzo depends on it I think the code should be migrated and maintained there, otherwise we have no means to guarantee its long term stability and it's a risk to anyone that depends on it. That said, the fix is trivial and doesn't affect us so we might as well ship it for now.

@clairekope our current plan is to wait for numpy 2.0.0rc to come out before we cut a new bug fix release. Currently this means that this patch won't be part of a public release for another month (give or take), is that okay for you ?

@neutrinoceros neutrinoceros added this to the 4.3.1 milestone Dec 21, 2023
Co-authored-by: Clément Robert <cr52@protonmail.com>
@clairekope
Copy link
Contributor Author

If this test code isn't used in yt itself we probably ought to remove it at some point, so if Enzo depends on it I think the code should be migrated and maintained there, otherwise we have no means to guarantee its long term stability and it's a risk to anyone that depends on it. That said, the fix is trivial and doesn't affect us so we might as well ship it for now.

I don't disagree; it's part of yt's legacy as a primarily Enzo toolchain. It's something I can try to bring up with the Enzo dev community. Do other frontends have such specialized testing tools hosted in yt?

@clairekope our current plan is to wait for numpy 2.0.0rc to come out before we cut a new bug fix release. Currently this means that this patch won't be part of a public release for another month (give or take), is that okay for you ?

Not a problem; we build from the tip of the git repo for CI testing.

@neutrinoceros
Copy link
Member

Do other frontends have such specialized testing tools hosted in yt?

Not that I'm aware of, but maybe @matthewturk is in a better position to answer that.

@neutrinoceros
Copy link
Member

@yt-fido test this please

@neutrinoceros neutrinoceros merged commit 42eb142 into yt-project:main Dec 21, 2023
13 checks passed
meeseeksmachine pushed a commit to meeseeksmachine/yt that referenced this pull request Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants