more SPH field improvements #4584

jzuhone · 2023-07-19T20:33:24Z

PR Summary

Continuing to find improvements to be made to detection of SPH fields:

Supporting Gadget fields such as "ColdFraction", "CloudFraction", and "HotPhaseTemperature" with yt-like aliases.
Support AREPO/TNG fields "cooling_rate" and "cooling_time".
Metal fields can be of "local" sampling_type or "particle" sampling_type depending on if they are star or gas particles.
The core of the PR is to make detection of metal fields much more flexible for Gadget and its derivatives, accounting for the (somewhat unfortunate) variety of naming schemes for how these fields are stored in files.

Tests, and probably a bit of documentation about how metal fields are detected, are incoming.

PR Checklist

New features are documented, with docstrings and narrative docs
Adds a test for any bugs fixed. Adds tests for new features.

matthewturk · 2023-07-20T22:27:27Z

yt/frontends/gadget/fields.py

                units=self.ds.unit_system["density"],
            )

        return metal_names

-    def _setup_eleven_metal_masses(self, ptype):
+    def _make_fraction_functions(self, ptype, fname):


Is it possible at all to reuse the code in periodic_table.py?

I'll try, but I actually don't think it works

matthewturk · 2023-07-20T22:42:47Z

yt/frontends/gadget/fields.py

+                ret = mass / data[(ptype, "particle_mass")]
+                return ret
+
+            def _h_fraction(field, data):


Just to clarify, this is done this way because it doesn't track H specifically?

That's right

matthewturk · 2023-08-22T16:52:18Z

What else are you thinking before moving to ready-to-review?

jzuhone · 2023-08-22T17:23:18Z

@matthewturk tests and docs, sorry, got sidetracked

jzuhone · 2023-08-22T20:48:04Z

This PR now depends on yt-project/website#122 and the corresponding file (already on dickenson) needs to be put on Jenkins (@Xarthisius)

neutrinoceros · 2023-09-05T08:11:49Z

@yt-fido test this please

jzuhone · 2023-09-05T12:56:40Z

Jenkins still needs the file, I believe, and then I will add the new answer test. Ping @Xarthisius for https://yt-project.org/data/magneticum_camels.tar.gz

jzuhone · 2023-09-08T20:34:29Z

@Xarthisius just wanted to ping you again on the above ^^ (sorry for being a nuisance)

Xarthisius · 2023-09-08T23:14:09Z

@Xarthisius just wanted to ping you again on the above ^^ (sorry for being a nuisance)

Sorry I missed it! It's up now.

jzuhone · 2023-09-09T03:42:38Z

Let's see if the new test passes here (it did on my laptop). If it does, we're ready to go here.

jzuhone · 2023-09-09T04:24:26Z

@yt-fido test this please

jzuhone · 2023-09-09T12:41:19Z

@Xarthisius Jenkins failures appear unrelated?

neutrinoceros · 2023-09-09T13:35:23Z

@jzuhone regular tests are a bit flaky since we switch to Python 3.10, which is what we're seeing here. And from what I understand, doc builds are unstable until #4663 is done.

jzuhone · 2023-09-10T16:36:43Z

@yt-fido test this please

jzuhone · 2023-09-11T22:23:24Z

@yt-fido test this please

jzuhone · 2023-09-12T00:56:17Z

This is ready to go.

munkm

This looks good! I have a few minor comments/questions.

munkm · 2023-09-12T10:38:26Z

yt/frontends/gadget/fields.py

+            n_elem = len(
+                [
+                    fd
+                    for fd in self.ds.field_list
+                    if fd[0] == ptype and fd[1].startswith("MetalMasses")
+                ]
+            )
+            elem_names = {
+                7: ["C", "N", "O", "Mg", "Si", "Fe", "Ej"],
+                8: ["He", "C", "O", "Mg", "S", "Si", "Fe", "Ej"],
+                11: ["He", "C", "Ca", "O", "N", "Ne", "Mg", "S", "Si", "Fe", "Ej"],
+                15: [
+                    "He",
+                    "C",
+                    "Ca",
+                    "O",
+                    "N",
+                    "Ne",
+                    "Mg",
+                    "S",
+                    "Si",
+                    "Fe",
+                    "Na",
+                    "Al",
+                    "Ar",
+                    "Ni",
+                    "Ej",
+                ],
+            }[n_elem]
+            no_He = "He" not in elem_names


I found the logic of this part a little bit confusing. But I don't have a good alternative suggestion.

I agree with you--I can add some comments to explain what's going on.

The new comments are helpful! Thank you for adding them.

munkm · 2023-09-12T10:41:50Z

yt/frontends/gadget/fields.py

+            elem_names = {
+                7: ["C", "N", "O", "Mg", "Si", "Fe", "Ej"],
+                8: ["He", "C", "O", "Mg", "S", "Si", "Fe", "Ej"],
+                11: ["He", "C", "Ca", "O", "N", "Ne", "Mg", "S", "Si", "Fe", "Ej"],


Out of curiosity, is there a possibility that an output will have both MetalMasses and ElevenMetalMasses fields?

Not really--both are in fact somewhat arbitrary. ElevenMetalMasses is a translation field name for Gadget binary datasets defined here. MetalMasses is a convention I've seen used in Gadget HDF5, and (even worse) Mass of Metals is one I saw used in another similar dataset.

Ok, cool! Thanks for the clarification.

munkm · 2023-09-12T10:42:43Z

yt/frontends/gadget/fields.py

+                    if fd[0] == ptype and fd[1].startswith("MetalMasses")
+                ]
+            )
+            elem_names = {


Maybe moving the elem_names dict outside of both field defs and using the same list of elements elem_names[11] for both ElevenMetalMasses and MetalMasses fields would reduce some of the redundancy in your code?

munkm · 2023-09-12T10:48:03Z

yt/frontends/gadget/tests/test_outputs.py

+    elems = [
+        "He",
+        "C",
+        "Ca",
+        "O",
+        "N",
+        "Ne",
+        "Mg",
+        "S",
+        "Si",
+        "Fe",
+        "Na",
+        "Al",
+        "Ar",
+        "Ni",


I noticed your MetalMasses_ definition didn't include a definition for 14. Did you want to add that too since that's the use case for your test?

So 14 has to be treated differently. It's not a real element, it's basically just the mass of the rest of the elements not specified in the list, and in the codes that use it it's called Ej for "ejecta" (because it comes from supernovae explosions). We define an Ej_fraction field, but the reason I can't stick it in the elem list is that list the Ej_mass field doesn't exist, as it's not a real element and it doesn't get passed through yt's species machinery (though maybe it should be defined on its own).

Ok so your new commits do define it on its own now, is my understanding of that commit correct?

neutrinoceros · 2023-09-12T13:32:00Z

yt/frontends/gadget/fields.py

+                15: [
+                    "He",
+                    "C",
+                    "Ca",
+                    "O",
+                    "N",
+                    "Ne",
+                    "Mg",
+                    "S",
+                    "Si",
+                    "Fe",
+                    "Na",
+                    "Al",
+                    "Ar",
+                    "Ni",
+                    "Ej",
+                ],


feels like a good place to use # fmt: skip

neutrinoceros · 2023-09-12T13:38:29Z

I meant to fill in as a reviewer but I'm happy to get behind @munkm's approval.

…uce redundancy

jzuhone · 2023-09-12T14:34:02Z

@munkm @neutrinoceros see c8f4604 and 5156e29, thanks for the helpful suggestions that influenced these.

jzuhone · 2023-09-13T20:30:40Z

This is ready.

munkm

I like the improvements you made. Thank you for the changes.

One thing I thought about overnight -- is there any possibility a file would have more than one grouping of n_elem? (that is, that more than one option could be triggered by your logic) I can't think of a situation where that would happen, but if it does it it something we should add handling for?

munkm · 2023-09-13T23:12:05Z

yt/frontends/gadget/tests/test_outputs.py

+    elems = [
+        "He",
+        "C",
+        "Ca",
+        "O",
+        "N",
+        "Ne",
+        "Mg",
+        "S",
+        "Si",
+        "Fe",
+        "Na",
+        "Al",
+        "Ar",
+        "Ni",


Ok so your new commits do define it on its own now, is my understanding of that commit correct?

munkm · 2023-09-13T23:13:24Z

yt/frontends/gadget/fields.py

+            elem_names = {
+                7: ["C", "N", "O", "Mg", "Si", "Fe", "Ej"],
+                8: ["He", "C", "O", "Mg", "S", "Si", "Fe", "Ej"],
+                11: ["He", "C", "Ca", "O", "N", "Ne", "Mg", "S", "Si", "Fe", "Ej"],


Ok, cool! Thanks for the clarification.

munkm · 2023-09-13T23:13:55Z

yt/frontends/gadget/fields.py

+            n_elem = len(
+                [
+                    fd
+                    for fd in self.ds.field_list
+                    if fd[0] == ptype and fd[1].startswith("MetalMasses")
+                ]
+            )
+            elem_names = {
+                7: ["C", "N", "O", "Mg", "Si", "Fe", "Ej"],
+                8: ["He", "C", "O", "Mg", "S", "Si", "Fe", "Ej"],
+                11: ["He", "C", "Ca", "O", "N", "Ne", "Mg", "S", "Si", "Fe", "Ej"],
+                15: [
+                    "He",
+                    "C",
+                    "Ca",
+                    "O",
+                    "N",
+                    "Ne",
+                    "Mg",
+                    "S",
+                    "Si",
+                    "Fe",
+                    "Na",
+                    "Al",
+                    "Ar",
+                    "Ni",
+                    "Ej",
+                ],
+            }[n_elem]
+            no_He = "He" not in elem_names


The new comments are helpful! Thank you for adding them.

jzuhone changed the title ~~Sph fields~~ more SPH field improvements Jul 19, 2023

jzuhone added the enhancement Making something better label Jul 19, 2023

matthewturk previously approved these changes Jul 20, 2023

View reviewed changes

jzuhone force-pushed the sph_fields branch from 46f4637 to 6487e47 Compare August 22, 2023 19:35

jzuhone mentioned this pull request Aug 22, 2023

Entry for magneticum_camels file yt-project/website#122

Merged

jzuhone dismissed matthewturk’s stale review via a7c8e61 August 22, 2023 20:48

jzuhone marked this pull request as ready for review September 4, 2023 21:14

neutrinoceros self-requested a review September 5, 2023 08:37

jzuhone added 10 commits September 11, 2023 17:02

Fix GIZMO sfr alias

1d2eb12

Add alternate names for these fields

e2cd9c5

Add cooling fields for Arepo

080a2b6

Allow Gadget's HDF5 I/O to check for metal masses this way

ab31704

More general checking of element mass fields

e59bc2b

Metallicity field

7168ae6

Remove print

c9f63fa

pre-commit

0fddc1b

Fix corner cases

fb8e119

Docs for Gadget species fields

159fb36

jzuhone added 2 commits September 11, 2023 17:02

Add "ejecta" fraction and density fields to the whitelist

ca22a23

Add a test for magneticum_camels

9c5a826

jzuhone force-pushed the sph_fields branch from 3e7f8a0 to 9c5a826 Compare September 11, 2023 21:02

munkm previously approved these changes Sep 12, 2023

View reviewed changes

neutrinoceros reviewed Sep 12, 2023

View reviewed changes

jzuhone added 2 commits September 12, 2023 10:31

Move the element list options to definitions to clean up code and red…

c8f4604

…uce redundancy

Explicitly add Ej_mass field

5156e29

jzuhone dismissed munkm’s stale review via 5156e29 September 12, 2023 14:32

munkm approved these changes Sep 13, 2023

View reviewed changes

neutrinoceros added this to the 4.3.0 milestone Sep 14, 2023

neutrinoceros merged commit 8ef1569 into yt-project:main Sep 14, 2023
13 checks passed

jzuhone deleted the sph_fields branch January 22, 2024 00:03

more SPH field improvements #4584

more SPH field improvements #4584

Conversation

jzuhone commented Jul 19, 2023 • edited

PR Summary

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewturk commented Aug 22, 2023

jzuhone commented Aug 22, 2023

jzuhone commented Aug 22, 2023

neutrinoceros commented Sep 5, 2023

jzuhone commented Sep 5, 2023

jzuhone commented Sep 8, 2023

Xarthisius commented Sep 8, 2023

jzuhone commented Sep 9, 2023

jzuhone commented Sep 9, 2023

jzuhone commented Sep 9, 2023

neutrinoceros commented Sep 9, 2023

jzuhone commented Sep 10, 2023

jzuhone commented Sep 11, 2023

jzuhone commented Sep 12, 2023

munkm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neutrinoceros commented Sep 12, 2023

jzuhone commented Sep 12, 2023

jzuhone commented Sep 13, 2023

munkm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jzuhone commented Jul 19, 2023 •

edited