-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add support for neutral free-free absorption involving some metals/molecules #111
base: main
Are you sure you want to change the base?
Conversation
(Let me know if you want me to pick this up from here, happy for you to do it if you prefer) |
…ng neutral metals and ions. - I added support for Ominus, COminus, and H2minus. - Most of the tooling is in place to support H20minus - I also added code for Nminus and Cminus, but a comparison against another source makes me a little worried about these. I've disabled this for now... Most/All of these are probably unimportant. They are mostly included just because Turbospectrum has them.
eee324b
to
4bfd9c3
Compare
Codecov Report
@@ Coverage Diff @@
## main #111 +/- ##
==========================================
+ Coverage 79.38% 79.52% +0.14%
==========================================
Files 18 19 +1
Lines 1072 1094 +22
==========================================
+ Hits 851 870 +19
- Misses 221 224 +3
Continue to review full report at Codecov.
|
src/ContinuumAbsorption/absorption_ff_neutral_metals_molecules.jl
Outdated
Show resolved
Hide resolved
src/ContinuumAbsorption/absorption_ff_neutral_metals_molecules.jl
Outdated
Show resolved
Hide resolved
src/ContinuumAbsorption/absorption_ff_neutral_metals_molecules.jl
Outdated
Show resolved
Hide resolved
src/ContinuumAbsorption/absorption_ff_neutral_metals_molecules.jl
Outdated
Show resolved
Hide resolved
src/ContinuumAbsorption/absorption_ff_neutral_metals_molecules.jl
Outdated
Show resolved
Hide resolved
src/ContinuumAbsorption/absorption_ff_neutral_metals_molecules.jl
Outdated
Show resolved
Hide resolved
and data from [John 1975, MNRAS, 172, 305](https://ui.adsabs.harvard.edu/abs/1975MNRAS.172..305J) | ||
for λ ≤ 10 μm. This is the approach recomended by the latter paper. The error on these absorption | ||
coefficients is probably large. | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this file should be restructured somehow to avoid the very similar doctrings. Do functions for individual molecules need to exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Combining these seems sensible to me. I'm inclined to leave the metals as separate functions since they all use data from different papers, but if you feel super strongly about this, I can probably be convinced
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems sensible to me. The tabulated data are in slightly different formats, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ominus_ff uses data from John 75a and John 75b.
- The format of John75a is pretty different from the formats of everything else - to make these quantities equivalent to the other sources, we just interpolate in T and then multiply the result by λ^2 (in units of Angstroms^2). This "common format" just requires that the computed "coefficient" is multiplied by the partial electron pressure and the number-density of the relevant neutral species.
- John 75a involves interpolating over λ and T
Even though they come from different papers, from each other the Cminus_ff & Nminus_ff data share the same format (Bell & Berrington were coauthors on both of those papers). This format is very similar to John 75b except they use θ instead of T (which obviously is an important distinction if we aren't interpolating in log-space).
So, if you felt it was important that we force everything into a single function, it's doable. It just doesn't seem like an obvious improvement (I really don't think we'll ever need to add any other negative atomic ion free-free absorption calculations beyond what we already have: Hminus_ff
, Heminus_ff
, Cminus_ff
, Nminus_ff
, and Ominus_ff
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I‘ll leave this to your judgement.
# A better test might use reference data from John & Williams 1977 | ||
@test assert_allclose(calc_α, ref_α; rtol = 6.8, atol =0.0); | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do the bell and berrington coefficients need tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was kind of trying to test _bell_berrington_ff_neg_ion_absorption
here by comparing Nminus ff data from Ramsbottom et al. 1992 against Nminus ff data from John 75a (that could be better documented).
With that said, I just thought of an obvious, more direct test that I could add for both Nminus_ff
and Cminus_ff
. I'll get to that in the next few days (and it will directly address my concerns about the factor of 8 discrepancy)
Co-authored-by: Adam Wheeler <[email protected]>
What's the status of this? Is it something you have time/inclination to work on or should I take it from here? A third option is to drop it until after the paper is re-submitted. |
At this point, I don't think I'll get back to this for a while. So the choice is totally up to you. I don't really think most of these source are important - so it doesn't really matter if you merge before publishing. But, my impression was that there wasn't much work to do to get this merged in. With that in mind, and for the sake of selling the code to more naive readers (who are unaware that these sources aren't important), this might be worth doing since it adds a bunch of sources that turbospectrum has. |
I was feeling motivated yesterday, so I decided to introduce support for some free-free absorption coefficients involving neutral metals and ions. Most/All of these are probably unimportant. They are mostly included just because Turbospectrum has them.
In detail:
EDIT: If you look at Table I of Gustafsson et al. 2008, they talk about using free-free H2minus data from John & Williams (1975). However, when I looked up that paper it doesn't say anything about H2minus (it just talks about Ominus ff absorption). I looked at citation names in the turbospectrum codebase and I think this must have been a typo in paper