-
Notifications
You must be signed in to change notification settings - Fork 276
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
Add support for Enzo-E simulations with 0 ghost zones #4932
Add support for Enzo-E simulations with 0 ghost zones #4932
Conversation
f50ed35
to
be493fa
Compare
626e74e
to
6ee41e8
Compare
Gotcha. FYI this is currently built off of a slightly older point in the main branch. This was mainly for convenience (I was a really hard time setting up yt off of the HEAD of the main-branch. My issues are probably user-error. (I recently switched to a new machine and started using conda for the first time in years). I'm happy to fast-forward this whenever. |
as long as there are no conflicts (which there aren't), this shouldn't matter :) |
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.
This looks fine to me, just a minor suggestion
@@ -406,9 +406,12 @@ def _parse_parameter_file(self): | |||
ablock = fh[list(fh.keys())[0]] | |||
self.current_time = ablock.attrs["time"][0] | |||
self.parameters["current_cycle"] = ablock.attrs["cycle"][0] | |||
gsi = ablock.attrs["enzo_GridStartIndex"] | |||
gsi = ablock.attrs["enzo_GridStartIndex"] # <- always has 3 elements |
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.
Instead of a comment, you could add an assert
here, which would be less likely to rot silently
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.
Good point! Fixed
self.ghost_zones = gsi[0] | ||
# Enzo-E technically allows each axis to have different ghost zone | ||
# depths (this feature is not really used in practice) | ||
self.ghost_zones = gsi |
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.
although technically not backward compatible, I don't expect anyone to rely on the old behavior so I'm tempted to just let this change in.
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.
I definitely agree, especially because pool of enzo-e users is currently very small. (At most, there are maybe 5 people who regularly use the yt-frontend right now)
But, let me know if you would prefer I change it (and whether there is an obvious preferred way to do it)
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.
I think it's fine as is. I'll just give it a couple weeks before merging in case anyone objects.
@yt-fido Test this please |
The typing error is unrelated and already fixed on main, so let’s get this in. Thanks again for your patience ! |
PR Summary
The primary purpose of this PR was to add support for Enzo-E simulations with 0 ghost zones.
While I was here, I added support for Enzo-E simulations where the x, y, and z axes have different numbers of ghost zones (this is a feature that Enzo-E supports, but it's not really used). I also added some explanatory comments.