ENH: Update Enzo-E particle mass/density distinction #3914
Conversation
…her particle is mass or density.
brittonsmith
added
code frontends
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
|
This is now ready for review. @stefanarridge has kindly provided me with new Enzo-E datasets that can be used for testing. I'm happy to add those at some point, though I'm not sure when the best time to do that is. |
There was a problem hiding this comment.
Here are a couple suggestions. Otherwise LGTM
@stefanarridge, feel free to explicitly approve this PR (or review it some more).
Thanks to both of you !
| mass_flag = nested_dict_get( | ||
| self.parameters, ("Particle", "mass_is_mass"), default=None | ||
| ) | ||
| self._particle_mass_is_mass = mass_flag is not None |
There was a problem hiding this comment.
I would prefer we avoid adding "optional" attributes. In this case the _particle_mass_is_mass attribute is only used in two places, so it seems preferable to just call nested_dict_get and fetch the parameter when needed.
There was a problem hiding this comment.
That's a fair position with respect to attributes on the dataset class, but I'd rather not have to evaluate this over and over. Enzo-E datasets can tons of blocks. I'll hang this off the io handler, which is not meant to be user facing.
There was a problem hiding this comment.
Fair enough. I like how you refactored it to avoid hasattr !
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
matthewturk
changed the title
PR Summary
Enzo-E has been updated to include a new way of distinguishing whether particles are saved as masses or densities. This is done using the "mass" or "density" parameter in the parameter file. Previously, particle masses were to be considered as densities if the simulation was cosmological. To preserve backward compatibility, Enzo-E now uses an "is_gravitating" group name that I use to tell if the data was produced before or after this change.
PR Checklist