Add functionality to allow user to modify abundances

[jwreep]

Fixes #237

This PR adds a setter for the abundance property on the Ion and Element objects such that a user can supply a custom value of the abundance when creating the object or set the abundance property after instantiation.

Please review, and check whether there's any additional logic worth adding.

Looks functional:

(base) C:\Users\reep\Documents\Forks>python
Python 3.11.5 | packaged by conda-forge | (main, Aug 27 2023, 03:23:48) [MSC v.1936 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import astropy.units as u
>>> import fiasco
>>> iron = fiasco.Element('iron', [1e4, 1e6, 1e8]*u.K)
>>> iron.abundance
<Quantity 0.00012589>
>>> iron.abundance = 1e-3
>>> iron.abundance
0.001
>>> iron[26].abundance
0.001
>>> exit()

(base) C:\Users\reep\Documents\Forks>python
Python 3.11.5 | packaged by conda-forge | (main, Aug 27 2023, 03:23:48) [MSC v.1936 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import astropy.units as u
>>> import fiasco
>>> iron = fiasco.Element('iron', [1e4, 1e6, 1e8]*u.K, abundance=1e-3)
>>> iron.abundance
0.001
>>> iron[26].abundance
0.001
>>> exit()

(base) C:\Users\reep\Documents\Forks>python
Python 3.11.5 | packaged by conda-forge | (main, Aug 27 2023, 03:23:48) [MSC v.1936 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import astropy.units as u
>>> import fiasco
>>> iron = fiasco.Element('iron', [1e4, 1e6, 1e8]*u.K, abundance='sun_photospheric_2007_grevesse')
>>> iron.abundance
<Quantity 2.81838293e-05>
>>> iron[26].abundance
<Quantity 2.81838293e-05>
>>> iron.abundance = 1e-3
>>> iron[26].abundance
0.001

[codecov]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (586984f) 91.90% compared to head (be09fbc) 92.00%.

Files	Patch %	Lines
fiasco/fiasco.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #258      +/-   ##
==========================================
+ Coverage   91.90%   92.00%   +0.09%     
==========================================
  Files          38       38              
  Lines        2718     2752      +34     
==========================================
+ Hits         2498     2532      +34     
  Misses        220      220              

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

[jwreep]

One caveat: the user can change the abundance of an individual ion without updating it for the whole element. I'm not sure whether that's worth fixing? That is,

>>> iron[25].abundance = 1e-3
>>> iron.abundance
<Quantity 0.00012589>

The abundance of an ion need not be the same for the other ions, or for the element. User beware!

This could be changed easily by updating all the ions simultaneously, if preferred.

[jwreep]

The missing abundance test is failing because I've instantiated abundance in the __init__ function. I'll have to think about how to fix this.

[wtbarnes]

Thanks for tackling this. I've left a few suggestions that I think will clear things up and also fix the test. This also needs a test for the case of instantiating an ion with a scalar value of the abundance. This has proved to be a little thornier than I thought!

The one complication of this that I didn't anticipate is how this affects _instance_kwargs. The _instance_kwargs property is just a convenience property for sort-of-serializing the input arguments used to create that instance, including the abundance information. As it currently stands in this PR, a custom abundance value will not be passed to a new instance created with _instance_kwargs because in that case, an entry is not created in self._dset_names. We could of course just add abundance to _instance_kwargs, but just doing this means information about abundance dataset (if any) was used. I would suggest adding a check in _instance_kwargs that checks if there is an abundance key in _dset_names and if there is, passes this value in as 'abundance' in _instance_kwargs and if not, just pass abundance. The logic that gets applied in the setter in the new instance then takes care of the rest. This behavior should also get a test.

[wtbarnes]

One caveat: the user can change the abundance of an individual ion without updating it for the whole element. I'm not sure whether that's worth fixing?

That's partially just the price we pay for using Python (i.e. no actually private variables). Although, honestly I'm not too worried here. An Element object is explicitly created as a collection of ions whose inputs (other than the ionization stage), are all the same. A user would have to explicitly set one abundance value in the Element to something different, i.e. it is hard to accidentally make that mistake.

We could add a check in Element.abundance that checked that all ions have the same abundance each time the abundance is accessed, but I worry that would just be adding latency for not much gain.

[jwreep]

I made one further change to the Element setter so that we can use a string value to change the element's abundance, which didn't work otherwise.

(base) C:\Users\reep\Documents\Forks\fiasco>python
Python 3.11.5 | packaged by conda-forge | (main, Aug 27 2023, 03:23:48) [MSC v.1936 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import astropy.units as u
>>> import fiasco
>>> iron = fiasco.Element('iron', [1e4, 1e6, 1e8]*u.K)
>>> iron.abundance
<Quantity 0.00012589>
>>> iron.abundance = 1e-3
>>> iron[26].abundance
0.001
>>> iron.abundance = 'sun_photospheric_2007_grevesse'
>>> iron[26].abundance
<Quantity 2.81838293e-05>
>>> iron.abundance
<Quantity 2.81838293e-05>

[wtbarnes]

Thanks for your patience with this. I think this PR just needs three more things:

1. Test for setting Ion.abundance with a string and float
2. Test for setting Element.abundance with a string and a float (The snippet of code in your above comment would more than suffice)
3. Fix for _instance_kwargs that I mentioned above.

I'm also happy to add these if you don't have time to do so.

[jwreep]

Right, lots to keep to track of! Hopefully this latest one covers the rest. I added tests for all three of these . . .

. . . and now I'm stumped. Why is it not finding the photospheric abundance set? Works just fine in the terminal.

[wtbarnes]

Why is it not finding the photospheric abundance set? Works just fine in the terminal.

It's because the tests are run on a minimal version of the database in order to reduce the amount of time spent building the HDF5 database every time the tests are run. When you run the tests on your machine, you're probably using your prebuilt version of the database. The full list of files inlcuded in the test database is in fiasco/conftest.py. I had previously added only the coronal abundances, but have now added the photospheric ones so those should pass now.

I also fixed up some of the tests that I had previously written that used the old abundance_filename key so I think everything should be working now.

[wtbarnes]

Thanks for your work on this and for your patience!