Small Arepo/Gadget enhancements #4419
Conversation
jzuhone
added
code frontends
| uvals["cmcm"] = 1.0 / uvals[unit] | ||
| else: | ||
| missing[i] = True | ||
| for grp in ["/Header", "/Parameters", "/Units"]: |
There was a problem hiding this comment.
I am completely stumped as to the Jenkins test failures. I cannot reproduce the difference on this PR vs. main on my own machine--even after switching to Python 3.8 and letting pip install all of the dependencies. Can someone help check the logic here to make sure I didn't do something silly?
There was a problem hiding this comment.
FWIW, both answers I get (this PR vs main on my machine) agree with the "new" ones
There was a problem hiding this comment.
^^ @Xarthisius any special Jenkins insight would be helpful
| return data["gas", "mass"] / data["gas", "density"] | ||
|
|
||
| self.add_field( | ||
| (ptype, "cell_volume"), | ||
| ("gas", "cell_volume"), | ||
| function=_volume, | ||
| sampling_type="local", | ||
| units=self.ds.unit_system["volume"], |
There was a problem hiding this comment.
Should this be changed elsewhere too? It looks reasonable to me, since in Arepo the volume (at least right now) will only make sense for the gas particles. Do we need to concern ourselves with potential two-fluid sims?
There was a problem hiding this comment.
By "this" you mean?
There was a problem hiding this comment.
I fixed this also now for the "pressure" field
| @@ -129,14 +129,26 @@ def _h_p1_fraction(field, data): | |||
| self.alias(("gas", field), (ptype, field)) | |||
|
|
|||
| if (ptype, "ElectronAbundance") in self.field_list: | |||
| # If we have ElectronAbundance but not NeutralHydrogenAbundance, assume the | |||
| # If we have ElectronAbundance but not NeutralHydrogenAbundance, | |||
There was a problem hiding this comment.
In practice will this change results for existing calculations?
There was a problem hiding this comment.
So I'm actually not convinced that we've ever encountered a corner case like this. I just noticed that this was a possibility recently when someone asked me about something related.
|
I can't figure out the test failures |
|
@yt-fido test this please |
|
This is ready to go. |
|
I'm confused, Matt didn't write a comment, so who approved bumping answers ? |
The reason was given in the updated description of this PR. |
|
ah, I missed it, thanks ! |
Co-authored-by: Clément Robert <cr52@protonmail.com>
|
Thanks @neutrinoceros |
|
thanks again all! |
PR Summary
This PR:
"cell_volume"field for Arepo datasets have"gas"type"star_formation_rate"an alias for"StarFormationRate"in Gadget and its derivativessampling_type="local"and not"particle")H_number_densityfield if there is noNeutralHydrogenAbundancefieldIn the course of testing this PR @Xarthisius and I found that Jenkins did not have the same version of the
TNGHalotest dataset as yt-project.org/data, hence also the answer bump.PR Checklist