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 method to change abundance of an element independent of sets in chianti.h5 #237

Closed jslavin closed 5 months ago

jslavin commented 1 year ago

I'd like to use fiasco to calculate a radiative loss curve for a plasma with depleted abundances. I've been able to change the abundance set by using the 'abundance_file' argument to an instantiation of an Element, but that's limited to the sets available in chianti.h5. As far as I can tell, there isn't another way to change the elemental abundances. It'd be nice if one could provide an array of abundances when instantiating an Element or else be allowed to change the abundance like element.abundance = X.

wtbarnes commented 1 year ago

I think this would be pretty easy to implement. In Element, the abundance information is actually just taken from the first Ion in the sequence so the best place to implement this is probably Ion.

The only minor complication I can think of is that when a custom abundance value is used, the abundance filename then becomes inaccurate. Additionally, it is ambiguous what a user wants if the abundance_filename and abundance value are both passed to Ion so there is some logic needed to deal with that. However, I don't think this should be too onerous.

jwreep commented 5 months ago

I was trying to implement this today, and running into a brick wall.

Normally, to create a setter so we can alter the stored values, it would be something like:

@abundance.setter
def abundance(self, abundance):
    self._abundance = abundance

And I kept getting an error about there not being a defined setter.

It looks like this is because ion._abundance stores a text description of the files in CHIANTI?

>>> iron[0]._abundance
fe/abundance -- v8.0.7

Fields
------
allen  -- abundance relative to H
cosmic_1973_allen  -- abundance relative to H
feldman  -- abundance relative to H

etc.

Perhaps there would be a better variable for this text information to be stored?

wtbarnes commented 5 months ago

tl;dr I agree and suggest renaming _abundance to _abund and using the setter in exactly the way you suggest above.

You're right to be confused as this is a clear case of my poor design choices years ago leading to a confusing API. The _abundance property is actually not a just a string descriptor of the abundance datasets but rather the data layer that provides access to the part of the database (it appears as a string because the the string representation of the object explicitly lists out the available datasets). There is a property like this for each file type within the database and is how fiasco manages accessing different parts of the database. These, including _abundance, are added on to IonBase which Ion inherits from.

The abundance is sort of a special case in that, unlike a lot of the parts of the dataset, it's something users want direct access to which is why there's both an _abundance and an abundance property. We should probably instead rename the former to _abund to make this distinction more clear.

Additionally, when it comes to passing in a value via the constructor, I would suggest doing away with the abundance_filename kwarg and instead rename it abundance and allow it to either be a float or a string. If it is a string, the abundance value is accessed in the usual way using the abund property. If it is a float, the abundance is set directly using that value.

wtbarnes commented 5 months ago

All of my above suggestions could also apply to the ioneq/_ioneq properties though that can be left to a separate PR.