Update classes & docs for ionization states and fix docstring formatting in plasmapy.particles #796
Conversation
namurphy
added
the
plasmapy.particles
|
I'm also wondering if it would be better to use the name |
|
I agree with renaming them. Having it be one letter of a typo away seems like a bad idea, come to think of them. |
There are still some problems with how IonicFraction is called that I will need to debug.
The name `IonizationStates` was too close to `IonizationState`. This renaming is to make sure that there are meaningful distinctions between variable names. My usage is that ionization state (singular) refers to a single element, whereas ionization states (plural) refers to multiple elements.
This comment has been minimized.
This comment has been minimized.
|
I just renamed I'm using ionization state (singular) to describe the ionic fractions for a single element, whereas ionization states (plural) refers to ionic fractions for multiple elements. |
When the ionic fraction or number density is not set, then it should default to numpy.nan (in appropriate units).
Codecov Report
@@ Coverage Diff @@
## master #796 +/- ##
=============================
=============================
Continue to review full report at Codecov.
|
Up until now, the IonizationState class has required the base particle
to be either an element or isotope. However, there may be some cases
where we want to set the ionic fraction to be 100% of a particular
ion. This pull request allows us to do things like
IonizationState("alpha"), in which case the base particle will be He-4
and the ionic fractions will be [0.0, 0.0, 1.0] (i.e., all He-4 atoms
are doubly charged).
Credit for the idea goes to:
Co-authored-by: Maurice Wilson <maurice.wilson@cfa.harvard.edu>
Also: MissingAtomicDataError to MissingParticleDataError
|
Following namurphy/PlasmaPy-NEI#1, I've started thinking that it would be worthwhile to rewrite the tests for
|
namurphy
changed the title
Yeah...I'm getting reticent about having |
I agree with that! I was thinking we'd have the discussion when I address issue #912 (and/or #918). I just wanted to point it out here. |
|
This is the issue I mentioned during the community meeting today, which would be really helpful to have merged by our 0.5.0 release. There are some breaking changes here, and having this merged will make it easier for me when I get back to working on the non-equilibrium ionization modeling affiliated package...hopefully in the second half of December. Thank you in advance! |
There was a problem hiding this comment.
LGTM, except for a few comments added below and https://github.com/PlasmaPy/PlasmaPy/pull/796/files?file-filters%5B%5D=.py#r541546319 (number density scaling factor argument name "n" is confusing)!
| except Exception as exc: | ||
| raise TypeError( | ||
| "Unable to ascertain equality between the following objects:\n" | ||
| f" {self}\n" | ||
| f" {other}" | ||
| ) from exc |
There was a problem hiding this comment.
Agreed with @rocco8773's previous review (github interface is once again messing up here) - this should probably just return False.
Co-authored-by: Erik Everson <eteverson@gmail.com>
Before this, __eq__ raised an exception. This change was suggested in code review.
The original kwarg name, `n`, did not satisfactorily convey that this was a number density scaling factor. `n0` does this better. If this too is insufficiently clear, then a future possibility would be `n_scale_fac`.
I also added the solar wind as an example use case in the narrative documentation.
|
Ok! I think I finished implementing the changes from code review, so if it looks good to you, feel free to merge. Thanks! |
This pull request adds narrative documentation for
IonizationStateandIonizationStatesand fixes some typos.