wtbarnes / fiasco

Python interface to the CHIANTI atomic database
http://fiasco.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
20 stars 15 forks source link

Add functionality to allow user to modify abundances #258

Closed jwreep closed 5 months ago

jwreep commented 5 months ago

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[bot] commented 5 months ago

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 :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jwreep commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

Thanks for your work on this and for your patience!