-
Notifications
You must be signed in to change notification settings - Fork 25
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
_ion_mass and _ion_number_density are not consistent #197
Comments
Hi Clayton. Thanks for the writeup. I can confirm that this appears to be an issue. I recall that when we wrote this originally, all the ion fields (mass, density, abundance, and number density) were directly tied to each other, such that this wasn't a problem. I think they potentially got tweaked with your PR from 4 years ago: PR #81 , which I think is the last to touch this functionality. I'll go through things and try to figure out the desired behavior (ie which field is correct). When I test this locally on the Mg II field, there is a fixed multiplicative offset between the two fields (density and density_from_mass) of 0.0433, which may be a hint, but I need to dig into a bit to figure out where things are going wrong. |
Wow! I had completely forgotten my part in writing this for #81 back in 2019! Well, in looking at some of @brittonsmith's comments there, I think the main reason I added this second sequence of priority checks is there is because we were not confident that SPH codes would have a "volume" we could multiply by density to get mass. One thing I've been doing with SPH codes in AGORA is letting
I don't think it necessarily matters whether |
Hi all,
I am working on a PR for a new feature allowing a user to specify what metal source they want to use for trident, in case they are comparing multiple codes which do not all retain the same metal species information. However I was struggling to write this PR because I couldn't figure out a clean way to integrate a custom metal function into both priority lists, in
_ion_mass
and_ion_number_density
. But as I was looking into it more, this is partially because having two separate priority lists that look for different fields doesn't really make sense as is.In my past use of trident, I had written a paper looking at OVI in an ART simulation. I had always only really used the
O_p5_number_density
field, and never had much need ofO_p5_mass
. Since ART keeps SNIa and SNII fields separate, I came up with an estimate O abundance that was better than solar, and I addedO_nuclei_mass_density
fields to theyt
ART frontend (yt-project/yt#1981). I didn't realize that this field gets used in the priority list for_ion_number_density
but NOT in the nearly-identical list in_ion_mass
, and so that field therefore uses solar abundances.This means, if I take the
O_p5_mass
field and divide by the volume, it makes something very different fromO_p5_density
, even though as a user I would naively assume them to be the same. The difference between them can be pretty dramatic:You can ignore the "agora_analysis" stuff, that's just basically wrappers for
yt.load
and containing the zoom-in center info, etc. This same effect will be true for any ART dataset, because my changes are in the frontend. But really, this will happen to any user who definesO_nuclei_mass_density
without definingO_nuclei_mass
or vice versa. Which I imagine could be pretty common.I guess I just want to ask if there's any interest in updating the logic of
ion_balance
to have only one priority list, and haveion_mass
be derived fromion_number_density
, likeion_density
currently is. This would force these fields to be consistent.I'm happy to write the PR to do that if so, but wanted to raise this as an issue regardless.
The text was updated successfully, but these errors were encountered: