Add new GauntFactor class and add Itoh integrated Gaunt factors to f-f radiative loss

[jwreep]

Fixes #248

• Parse the new files
• Implement new GauntFactor class
• Add Itoh options to total f-f Gaunt factor
• Add Itoh options to f-f radiative loss function
• Tests

[jwreep]

For my own reference, the equations from Itoh et al 2002 for the two forms of the Gaunt factor. (They're using log base 10.)

Relativistic:

Non-relativistic:

[wtbarnes]

As we implement this, I'd like to think carefully about how these are related to the other Gaunt factor methods. Currently, we have the following methods for calculating Gaunt factors:

• gaunt_factor_free_free--function of wavelength and temperature
• _gaunt_factor_free_free_itoh
  • used by gaunt_factor_free_free in the relativistic regime
• _gaunt_factor_free_free_sutherland
  • used by gaunt_factor_free_free in the non-relativistic regime
• _gaunt_factor_free_free_total
  • function of temperature
  • wavelength-integrated version of _gaunt_factor_free_free_sutherland (I think)
  • used by free_free_radiative_loss
• _gaunt_factor_free_bound_total
  • function of temperature
  • used by free_bound_radiative_loss

In our current organization of the API, even though most of these are private methods, it can be confusing why we have so many methods named very similar things and why/where each is relevant. While implementing these new methods, whether there is some better top level API for calculating these such that we minimize the number of entry points for calculating a Gaunt factor and make it more obvious to a user (or future developer, including ourselves, why method is preferred over and another).

[jwreep]

I'll spend a bit of time thinking about this. Open to suggestions!

[jwreep]

Unless I'm missing something, the actual data that's read in by fiasco.io for every type of Gaunt factor does not depend on the properties of an ion. That is, the fitting coefficients are the same for all ions.

If that's correct, perhaps it would be better to create a new module or class just dedicated to the Gaunt factor, and pull these functions out of the ion class. Then, call some overarching function along the lines of gaunt_factor(free_free=True, free_bound=False, wavelength_integrated=False) that then calls the appropriate function from the class.

This is trivial for free-free emission. For example, ion._gaunt_factor_free_free_itoh doesn't depend on any ion properties, ion._gaunt_factor_free_free_sutherland only depends on charge state and thus gives the same Gaunt factors for He III, Fe III, etc. at a given temperature.

The free-bound case is slightly more involved, because it depends on charge, ionization potential, and principal quantum number. However, nothing about this needs to be inherently tied to the ion class and each could be passed to a function.

Would something along those lines make sense?

[codecov]

Codecov Report

Attention: Patch coverage is 75.00000% with 92 lines in your changes missing coverage. Please review.

Project coverage is 90.46%. Comparing base (6642b07) to head (ee77037).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
fiasco/io/sources/continuum_sources.py	43.29%	55 Missing ⚠️
fiasco/gaunt.py	86.20%	24 Missing ⚠️
fiasco/tests/test_gaunt.py	88.52%	7 Missing ⚠️
fiasco/tests/test_collections.py	66.66%	3 Missing ⚠️
fiasco/tests/test_ion.py	66.66%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #293      +/-   ##
==========================================
- Coverage   93.14%   90.46%   -2.68%     
==========================================
  Files          38       40       +2     
  Lines        3165     3367     +202     
==========================================
+ Hits         2948     3046      +98     
- Misses        217      321     +104     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jwreep]

@wtbarnes, before I take this much further, your input would be welcome. I've created a new GauntFactor class with a bit of logic that should hopefully be readable code. It doesn't (shouldn't) affect anything outward-facing, but it does nest the various functions that calculate the different Gaunt factors into one object. Would something along these lines be a reasonable way to deal with the convoluted mess of Gaunt factors? Or did you have something else in mind?

Edit: also, I was wrong in my previous comment that some of the Gaunt factors don't depend on any ion properties. The Ion._itoh data is attached to an ion, though some of the other data is not.

(It still needs error handling and debugging, of course. It's a bit clumsy with .gf to get the actual values at the end as well.)

[wtbarnes]

Sorry for neglecting this for so long!

tl;dr I'm pro this approach overall. I have some thoughts on how exactly to implement this object. I've left a few high-level comments below and some inline comments on the code.

At first I was a bit skeptical of having these on a separate object, but I'm now thinking this is probably a good idea. Exposed as a separate object, they can still use the same data ingestion model as Ion which means we don't have to change anything at the I/O layer.

This also helps to shorten the ions.py file which seems to be ever expanding.

Edit: also, I was wrong in my previous comment that some of the Gaunt factors don't depend on any ion properties. The Ion._itoh data is attached to an ion, though some of the other data is not.

We could always change this in the way the parsing is done to be agnostic of Ion and then select the row appropriate to the given ion when doing the necessary calculation in the gaunt factor. I think it is per element anyway so it doesn't even need the granularity of per ion.

[wtbarnes]

Some implementation comments at this point. My overall thinking is that GauntFactor should not care about the Ion object at all and should just provide the relevant datasets and methods for computing the gaunt factor in various scenarios. It is then the job of the Ion object to provide the ion-specific inputs to those methods when using the appropriate gaunt factor calculation.

[jwreep]

Thanks for the suggestions! I'll try to flesh this out a bit more.

[jwreep]

@wtbarnes, has something changed with the github actions in the recent pulls? Pytest is passing on my machine, but failing on Github (including lots of tests not related to this PR).

[jwreep]

Thank you for the clarification, Peter!

I'll add a citation to this technical report in fiasco.

[wtbarnes]

Looking at the docs, I now realize that the API docs for the new GauntFactor object aren't rendered anywhere because I asked for it to be excluded from the top-level namespace. It probably makes sense to just add it back to the top-level namespace. Sorry!

[jwreep]

Yeah, that probably explains why the docs build failed. lol

[wtbarnes]

I also take back what I said about only needing this in the context of the Ion object. It's not unreasonable one might want to calculate a Gaunt factor for other reasons as well so making the API more visible is only a good thing.

[jwreep]

I think that should've addressed all of your comments! Let me know if there are any more changes you'd like.

[wtbarnes]

I think that should've addressed all of your comments! Let me know if there are any more changes you'd like.

Everything is looking pretty good to me! i just pushed a few changes mostly cleaning up the docstrings, linking between methods, and enforcing some consistency between a few of the equations. Have a look over the rendered docs for GauntFactor and relevant methods on Ion and let me know what you think. I think this should be basically done.

[jwreep]

Looks fine to me. Thanks for touching that up.

[wtbarnes]

This looks good to me! Thanks as always. I like the way this has turned out and make the Gaunt factor calculations much more readable and maintainable.

[jwreep]

Thanks for the help! This ended up being a lot more involved than just parsing two new data sets. Hopefully the new format is easy to maintain and extend. For example, the CHIANTI technical report mentions a newer data set that's more accurate, that could potentially be readily built in to fiasco.