-
Notifications
You must be signed in to change notification settings - Fork 271
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
ENH: Improved Enzo-E frontend's determination of fluid properties #4099
ENH: Improved Enzo-E frontend's determination of fluid properties #4099
Conversation
While I was doing this I tweaked some logic to more correctly handle some corner cases.
…ernal energy field is not saved to disk
…re used in a simulation. This is necessary for some upcoming changes.
…ic derivation of some derived fields (when the simulation used the dual-energy formalism)
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
yt/frontends/enzo_e/misc.py
Outdated
@@ -123,6 +123,18 @@ def nested_dict_get(pdict, keys, default=None): | |||
return val | |||
|
|||
|
|||
def get_listed_subparam(pdict, parent_param, subparam, default = None): |
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.
Do you think this potentially has use outside of enzo-e?
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 doubt it, since this is mostly used to address edge-cases that arise from a common pattern that occurs while enzo-e parses certain types of parameter groups. I suppose it's possible that similar patterns could arise for other frontends, but it seems unlikely.
To be safe, I'll briefly walk through the use-case of this function. A common pattern occurs in Enzo-E parameter files where the name of a group of parameters needs to appear in a subparameter called list
for the parameters in that group to actually be meaningful. To be concrete, consider the following 2 snippets that shows that show this pattern:
- This first example configures a simulation with hydro and gravity. The parameters in the
Method:mhd_vlct
group affect the simulation since"mhd_vlct"
appears in theMethod:list
subparameter.
Method {
list = [ "pm_deposit", "gravity", "mhd_vlct", "pm_update"];
mhd_vlct {
mhd_choice = "no_bfield";
dual_energy = true;
# Other parameters...
};
# Configurations for other methods...
}
- This second example configures a simulation with just gravity. All parameters in the
Method:mhd_vlct
group are meaningless since"mhd_vlct"
is not included in theMethod:list
subparameter.
Method {
list = [ "pm_deposit", "gravity", "pm_update"];
mhd_vlct {
mhd_choice = "no_bfield";
dual_energy = true;
# Other parameters...
};
# Configurations for other methods...
}
Calling get_listed_subparam(pdict, "Method", "mhd_vlct", default = "dummy")
will return:
- the dictionary of all parameters for the "mhd_vlct" subgroup.
"dummy"
; this outcome is the identical to scenario where no"mhd_vlct"
parameters are specified at all.
Looks alright to me, and this was already approved by @matthewturk so I'll get this on track for merging: |
for more information, see https://pre-commit.ci
PR Summary
The primary purpose of this PR is to modify the way that the Enzo-E frontend determines generic fluid properties (namely the adiabatic index and whether the internal_energy field is an alias of the
('gas', 'specific_thermal_energy')
field). This is necessary to reflect a recent change in how this information is specified in Enzo-E's parameter files (this information is now a lot more centralized than it used to be).This also introduces a handful of minor tweaks.
('gas', 'specific_thermal_energy')
field. This new approach is more robust('enzoe', 'internal_energy')
as('gas', 'specific_internal_energy')
instead of('gas', 'specific_thermal_energy')
. In certain cases, this error prevented the automatic creation of fluid fields like('gas', 'pressure')
.As an aside, I wanted to highlight a helper function called
get_listed_subparam
, and I'm really on the fence about whether it's necessary. On the one hand it's short and we only call it in 2 locations, but on the other hand I think it significantly simplifies the logic (and the line-count) in the two places that we call it. Let me know if you want me to drop it (or rename it)...PR Checklist
I'm not really sure if I should be introducing any new tests. Doing that would probably require us to introduce a new test dataset. Please let me know if you have any suggestions.