IonicFraction refactor #1046
IonicFraction refactor #1046
Conversation
StanczakDominik
added
plasmapy.particles
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1046 +/- ##
==========================================
- Coverage 96.89% 96.89% -0.01%
==========================================
Files 70 70
Lines 6920 6918 -2
==========================================
- Hits 6705 6703 -2
Misses 215 215 ☔ View full report in Codecov by Sentry. |
StanczakDominik
force-pushed
the
ionicfractionrefactor
branch
from
e508a3f to
f1ba7f9
Compare
StanczakDominik
changed the title
There was a problem hiding this comment.
Thank you again for doing this!
My main comment is that we should keep the same name for the attributes originally named ionic_fraction, since the ionic fraction is the fraction of the atoms of an element that are at that ionic level. Renaming the IonicFraction class to IonicLevel is what we want to do, since IonicLevel contains the ionic fraction as well as some other properties. For an example of the naming (at least for the way heliophysicists tend to use it)... He 1+ is the ionic level. If half of the helium is at that ionic level, then the ionic fraction would be 0.5.
If you request another review then I should be able to get to this pretty quickly.
| ionic_fraction: real number between 0 and 1, optional | ||
| ionic_level: real number between 0 and 1, optional |
There was a problem hiding this comment.
For the attribute, I'd keep the name as ionic_fraction since it's the fraction of the element/isotope that's at this ionic level.
| @@ -166,7 +164,7 @@ class IonizationState: | |||
| an element, isotope, or ion; or an integer representing the | |||
| atomic number of an element. | |||
|
|
|||
| ionic_fractions: `~numpy.ndarray`, `list`, `tuple`, or `~astropy.units.Quantity`; optional | |||
| ionic_levels: `~numpy.ndarray`, `list`, `tuple`, or `~astropy.units.Quantity`; optional | |||
There was a problem hiding this comment.
| ionic_levels: `~numpy.ndarray`, `list`, `tuple`, or `~astropy.units.Quantity`; optional | |
| ionic_fractions: `~numpy.ndarray`, `list`, `tuple`, or `~astropy.units.Quantity`; optional |
This also should be changed back to ionic_fractions.
There was a problem hiding this comment.
The argument, but not the field of IonizationState, if I understand correctly?
There was a problem hiding this comment.
Oh, okay, I get it now. Sorry, tired.
|
Gah, you're of course right; that's just overzealous grepping. Getting to it. |
Co-authored-by: Nick Murphy <[email protected]>
This reverts commit 04af38f.
|
That should do it! Sorry I didn't pay enough attention to this. |
Starting to apply changes to
IonicFractionfrom#1020 (reply in thread). Refactoring the implementation a little along the way. Part of #1059.
docs for T_iwill go in at a future time, along with that feature!