Skip to content
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

Metal lookup options #198

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

claytonstrawn
Copy link
Contributor

TRIDENT creates ion fields by defining functions in order, creating first ion_fraction, then ion_number_density, ion_density, and finally ion_mass. Ion fraction is arguably the main functionality of trident, looking up and interpolating a cloudy database table, with the others being more or less yt bookkeeping. However, this bookkeeping downplays the other dependency for ion number densities -- the number density of the underlying nuclei.

By default, trident checks for metals whether there is an existing <atom>_nuclei_mass_density field, then a <atom>_metallicity field, then finally a simple metallicity field, assuming solar abundances. Because of a bug with demeshening (#81) almost the same selection code is repeated in both ion_mass and ion_number_density (but, this means those might not be consistent with each other, as seen in #197). For hydrogen and helium, it goes through a similar but somewhat shorter selection process.

With the AGORA project, I had a unique task of comparing several different codes to each other which use different yt frontends, and which all have different fields available. For the sake of comparing them on as equal of footing as possible, I have been attempting several strategies to force trident to use a specific "metallicity" field, instead of using their individually most optimized source, and I found this hard to do. But it's actually worse than just forcing it to use metallicity, because I've defined my own field agora_metallicity which has been checked against each code and verified, whereas internal metallicity fields are sometimes incorrect or are weirdly multidimensional.

Trident has no mechanism for "skipping" fields or using custom ones, and yt has no reliable way to delete the higher-priority fields. I saw @chummels has looked into field deletion before in yt-project/yt#2131, but that didn't seem to go anywhere. I personally added the <atom>_nuclei_mass_density fields to ART in yt-project/yt#1981, and now I am trying to intentionally neglect these fields.

So, here's my solution: I added a new metal_source variable, to specify what fields to use as the "source" of that atom. By default, it uses the "best" source for each, and goes through the same priority logic as before, however this is now done in a new function _determine_best_source. However, you can specify a particular priority, or if you have your own nonstandard field to access, like "agora_metallicity", pass in "custom" and give a new function to use. There are three main changes to this PR.

  1. Adding this optional variable metal_source to add_ion_fields and the other functions it calls. Even though only add_ion_number_density actually uses this, since the functions call one another (though maybe this should change, see point 2), they all need access. This means that _ion_number_density is now a nested function inside a generator, which will decide for itself on the "source" to use if "best" is passed in. Also, added docstrings explaining this, and logging to the user.
  2. Tweaking the logic of add_ion_fields to explicitly call all four field functions, instead of calling one, which calls another, which calls another, etc. This just makes the code more readable. Instead of add_ion_mass always calling add_ion_density, it just checks if the ion density field exists, and calls it if it doesn't.
    (However, in my opinion it would be a good idea to consider removing this functionality entirely, and requiring users to use the preferred method add_ion_fields instead of using these individual functions. This way, each field can depend on the prior ones but doesn't have to keep all the info to create them. But, this change would require lots of advance warning in case users rely on those functions)
  3. Solving issue _ion_mass and _ion_number_density are not consistent #197 (incidentally, by undoing my prior changes in fixing error in _ion_mass #81) to force _ion_mass to use _ion_density as its source, instead of stepping through the priority list again. In my experience, this effective_volume trick works equally well for SPH and AMR codes, though any info people have on this would be appreciated.

@coveralls
Copy link

coveralls commented Apr 10, 2023

Pull Request Test Coverage Report for Build 8f10cd15-bfa7-48a4-8ac0-e17d6455a69b

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 40 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.09%) to 75.577%

Files with Coverage Reduction New Missed Lines %
/home/circleci/trident/trident/ion_balance.py 40 82.88%
Totals Coverage Status
Change from base Build b3b725de-e97a-4358-8225-f0648498ecac: -0.09%
Covered Lines: 1767
Relevant Lines: 2338

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants