tardis-sn/carsus

use `ion_charge` instead `ion_number` consistently throughout code

epassaro opened this issue · 5 comments

Thanks for the detailed explanation. We should make sure that this is consistent throughout all of the code. @wkerzendorf, @afloers and myself like ion_charge better than ion_number (since it is easier to interpret). Can you make an issue that we want to change to "ion_charge" also in the rest of the code?

Originally posted by @chvogl in #191 (comment)

I think the problem is a bit worse than I expected. ion_charge was supposed to be 1-based index while ion_number should be 0-based. For some reason I couldn't keep track of what Misha did, at some point both names are used indistinctly for 0-based quantities.

See: https://github.com/mishinma/carsus/blob/99f1a6cf1fd049080dc86afbbbdf90b8f68b9cc6/carsus/model/tests/test_schema.py

Seems ion_charge is what we call "Carsus notation". I think we're not using consistently throughout the code but this is lost in the output module where ion_number is zero-based.

For some reason, the SQL NIST parser used ion_number (but consistently: 1-based as it should). From cross-sections PR onwards the "new" non SQL parser uses ion_charge as the rest of the parsers.

I repeat, this is just a note, everything is consistent.

EDIT: this is not entirely true but it's still ok. The comparison I was made failed because compared only the prepared attributes (affected by the +1 described in the next comment).

Ok, seem it was in front of my nose all this time!

        Notes
        ------
        In TARDIS `ion_number` describes the final ion state, e.g. H I - H II
        is described with ion_number = 1 On the other hand, in carsus
        `ion_number` describes the lower ion state, e.g. H I - H II is
        described with ion_number = 0 For this reason we add 1 to `ion_number`
        in this prepare method.
        """

See: https://github.com/tardis-sn/carsus/pull/111/files

Summary:

  • ion_charge 0-based index
  • ion_number 1-based index
  • consistent all across the code with exception of the old (SQL) output module (and in consequence on the new too) to make atomic files compatible with TARDIS.

The new output module should require some refactor to apply this logic in a clean way. Also requires documentation.