-
Notifications
You must be signed in to change notification settings - Fork 279
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: Store the Enzo-E version number in parameters attribute of EnzoEDataset #4097
ENH: Store the Enzo-E version number in parameters attribute of EnzoEDataset #4097
Conversation
More recent versions of Enzo-E record the version number in the outputs. This change now stores that in the parameters attribute of EnzoEDataset instances (the value is associated with 'version_string'). This change also updates the check for whether Enzo-E particle masses should be interpretted as masses or densities to always interpret them as masses if a version string is specified. The previous way Enzo-E specified this choice (i.e. by appending a parameter called "mass_is_mass" to the output copy of the parameter file) ended up causing some problems on certain platforms.
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.
Thanks for your contribution ! I have one comment that I would like to see addressed, and a question that may not be relevant. Otherwise, this looks good to me !
# particle parameter | ||
mass_flag = nested_dict_get( | ||
self.ds.parameters, ("Particle", "mass_is_mass"), default=None | ||
) | ||
self._particle_mass_is_mass = mass_flag is not 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.
I know you haven't touched this line here but would it make more sense as follows ?
self._particle_mass_is_mass = mass_flag is not None | |
self._particle_mass_is_mass = bool(mass_flag) |
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.
The way it currently reads just checks for the existence of the parameter. For better or worse, the way that parameter historically got set was unsound and could set the parameter to a random value. This includes a value of zero. So I think we need to keep the current logic.
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.
Alright that makes sense to me. If you have a minute to add this information as a comment I think that'd be very valuable :)
… instance attribute of EnzoEDataset: "version_string" -> "version"
…ce of mass_is_mass parameter rather than checking the value.
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
Ran into a flaky test, let's run it again |
Same error. Third time's the charm ? @yt-fido test this please |
More recent versions of
Enzo-E
record the version number in the outputs. This PR now stores that in theparameters
attribute ofEnzoEDataset
instances (the value is associated with 'version_string').The primary reason that
Enzo-E
now saves this version number is to make it easier to interpret the data, when the interpretation changes in newer versions.At present, the version string only affects one thing: the distinction between particle masses and densities.
Enzo-E
started to save particle masses such that they should be interpreted as masses (previously they were usually treated as densities).Enzo-E
append a parameter called"mass_is_mass"
to the output copy of the parameter file (PR ENH: Update Enzo-E particle mass/density distinction #3914 started querying of this parameter).Let me know if you need me to add any new tests for this PR.