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

Fix for 2D AMReX datasets in which 3 values are in the amr.n_cell entry in the job_info file. #3445

Merged
merged 2 commits into from Jul 21, 2021

Conversation

atmyers
Copy link
Member

@atmyers atmyers commented Jul 20, 2021

The WarpX team has noticed that since the yt-4.0 release, some of our 2D CI jobs have been failing. To reproduce this, see the attached script and dataset. ds.domain_dimensions should be [64, 64, 1], but it is now shown as [64, 64, 64].

I think this was introduced in this commit, which made changes to how 1d and 2d amrex datasets are set up. The issue is that we sometimes use inputs files that work for both 2D and 3D simulations. Hence, the amr.n_cell entry to warpx_job_info may have three values in it, even if the dataset is 2D.

This PR seems to solve the issue for us. The fix was just to use ds.dimensionality to cap the number of entries used from the job_info file.

import yt
ds = yt.load("diag100001")
print(ds.domain_dimensions)  # should be [64, 64, 1]

diag100001.tar.gz

@atmyers atmyers added bug code frontends Things related to specific frontends labels Jul 20, 2021
@atmyers
Copy link
Member Author

atmyers commented Jul 20, 2021

The test that is failing is another 2D dataset where on main the domain_dimensions are incorrectly set to 3D - is it possible that the answers to the test were bumped by mistake?

@neutrinoceros
Copy link
Member

Hi Andrews ! Sorry for authoring the culprit commit, and thank you for catching it !
I wasn't aware of this subtlety in AMReX, but the fix makes sense to me.

is it possible that the answers to the test were bumped by mistake?

I don't think so, or at least I cannot understand how that would have happened atm. I think that what we're seeing is in fact that these specific answers were never correct in the first place.

Copy link
Member

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

I'll formally approve of this patch, and I bumping the answers as well

@atmyers
Copy link
Member Author

atmyers commented Jul 20, 2021

Thank you for looking at this so quickly! I have bumped the answers.

@neutrinoceros neutrinoceros merged commit ba44836 into main Jul 21, 2021
@neutrinoceros neutrinoceros deleted the amrex_dim_fix branch July 21, 2021 08:17
@neutrinoceros neutrinoceros mentioned this pull request Oct 12, 2021
42 tasks
@matthewturk
Copy link
Member

@meeseeksdev backport to yt-4.0.x

@lumberbot-app
Copy link

lumberbot-app bot commented Oct 15, 2021

Can't Dooooo.... It seem like this is already backported (commit is empty).I won't do anything. MrMeeseeks out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug code frontends Things related to specific frontends
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants