Skip to content

Create classes to represent ionization state distributions - #404

Merged
namurphy merged 103 commits into
PlasmaPy:masterfrom
namurphy:ionization-state
Oct 31, 2018
Merged

namurphy merged 103 commits into
PlasmaPy:masterfrom
namurphy:ionization-state

Conversation

@namurphy

Copy link
Copy Markdown
Member

My plan for this PR is to create classes to represent the ionization state distributions of one or more elements. I am going to add in a bunch of dunder methods like __getitem__ and maybe __call__ to help making access to the ionization states more straightfoward and intuitive. Any suggestions on the naming convention will be helpful so that we can maximize readability.

Eventually we'll need a way to calculate ionization state distributions assuming collisional ionization equilibrium, but that will be for a different PR. The purpose of this PR is to set up how to store and access the ionization distributions. This will be discussed in #352.

This will address some of #352. It will probably be best to wait until after the 0.1.0 release to merge this, since this PR is only for a partial implementation anyway.

@namurphy namurphy added Enhancement plasmapy.particles Related to the plasmapy.particles subpackage status: in progress labels Apr 24, 2018
@namurphy namurphy added this to the v0.2 milestone Apr 24, 2018
@pep8speaks

pep8speaks commented Apr 24, 2018 •

Copy link
Copy Markdown

Hello @namurphy! Thanks for updating your pull request.

Line 458:27: E202 whitespace before ')'
Line 460:5: E112 expected an indented block

Comment last updated on August 07, 2018 at 18:59 Hours UTC

@lemmatum lemmatum left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good start! Needs some more basic functionality prior to merge.

Comment thread plasmapy/atomic/ionization_states.py Resolved
Comment thread plasmapy/atomic/ionization_states.py Resolved
Comment thread plasmapy/atomic/ionization_states.py Resolved
Comment thread plasmapy/atomic/ionization_states.py Resolved
Comment thread plasmapy/atomic/ionization_states.py Resolved
@StanczakDominik
StanczakDominik force-pushed the master branch 4 times, most recently from 02e758d to 749a6bd Compare April 29, 2018 16:04
@namurphy namurphy self-assigned this May 1, 2018
namurphy added 15 commits May 1, 2018 12:44
I extended the docstring, clarified some NotImplementedErrors,
implemented slicing, set __repr__, and slightly extended tests.
I added a bunch of attributes and methods plus a bunch of checks, and
I think allowed this to be iterated.  I'm still not sure about what
the best names are for the different attributes, and a lot of what is
included in this so far is up in the air.  I'm not sure if we want to
include a way to calculate collisional ionization equilibrium since
that would require a lot of data to be stored by the package somehow,
but that would be really useful and something that would probably be
used a lot by plasma physicists.
I added the capability to iterate over charge states, in which case I
used a `collections.namedtuple` with different attributes for the
integer charge, ionic fraction, and ionic symbol since these are the
most relevant values.  It might end up being reasonable to include the
number density in here too, if available.

Prior to this, I had set separate `rtol` and `atol` keyword arguments
and attributes for use with numpy.isclose.  I changed this to a sole
`tol` attribute because (1) for checking the normalization, absolute
and relative tolerances reduce to the same thing because the relative
tolerance is relative to one, and (2) for tiny abundances, the
absolute tolerance will be more relevant than the relative tolerance.
In calls to numpy.isclose, I explicitly set `rtol` equal to zero so
that it's clear that we're dealing with absolute tolerances and not
relative tolerances.

I defaulted `tol` to 1e-15 since at 1e-16 I got errors with
np.isclose.
Before this, I had IonizationState[value] return just a float for the
ionization fraction.  Now it returns a namedtuple with the integer
charge, ionic fraction, and ionic symbol.  This change is because the
integer charge, ionic fraction, and ionic symbol are all likely to be
needed, and to be consistent with the behavior as an iterator.

It may be worth changing this namedtuple (presently named State) into
a class.  There's also a need to create an IonizationStates class that
includes more elements.
I noticed that using isinstance(n, int) was returning False when n was
a numpy.int64 object, for example.  On the World Wide Web™ I found we
could instead do isinstance(n, (int, numpy.integer)).  I made this
change in a bunch of atomic modules for Z and mass_numb.

I did not yet change the docstrings or type hints to reflect that
numpy.integer types are allowed, and am debating whether or not it
would be best to do so.  The docstrings and type hints will be more
explicit with allowed types if we say that numpy.integer types are
allowed, but the docstrings and especially the type hints will get
more difficult to read.  It would be helpful if we could make this
notation more compact.  If we end up checking type hints with mypy, we
might need to at least update the type hints.
This class holds ionization state distributions for multiple elements,
unlike IonizationState which holds ionization state distributions for
just a single element.  I started adding attributes and docstrings,
including a way to input the ionization state data.
@namurphy
namurphy force-pushed the ionization-state branch from a78cc8f to 097f899 Compare May 14, 2018 20:23
 - I created IonizationState.info as a function to print out quicklook
   data to the screen.  I changed the formatting of this to avoid
   <BLANKLINE> repeatedly showing up in the docs; instead we have
   64 * '-'.

 - I added tests and fixed bugs related to setting densities in
   IonizationStates, in particular when the abundances and/or the
   number density was not set.

 - I reordered the attributes and methods in both classes again to
   have ionic_fractions and related stuff just after the dunder
   methods.  These are pretty important and so I think they should be
   near the top.

 - I removed some docstrings from dunder methods since they were
   redundant with what was in, for example, the error message that
   showed up.  Plus, they were not appearing in the docs or in help.

 - I updated the docstrings somewhat for both.  However, there remains
   work to do updatting the class docstring to describe iteration,
   getting/setting items, etc.  The docstrings right now are
   incomplete.
@namurphy

Copy link
Copy Markdown
Member Author

The docstrings still need to describe the consequences of dunder methods and it'd be good to fill in a few more gaps in text coverage, but I am ready for this to be merged. After then, I can create separate issues for the remaining tasks.

@StanczakDominik

Copy link
Copy Markdown
Member

I'd just do one final round of review and likely merge this today - but what of the commit messages? I could try to trim some of it down to something more manageable. Is that okay?

@namurphy

Copy link
Copy Markdown
Member Author

I was thinking that it'd be easiest if I did the squash merge after you approve it or I make any final changes. That way I can remove commits that are now extraneous and maybe add some context to the remaining commits. Does that sound good?

@StanczakDominik StanczakDominik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell (moving stuff around makes stuff more difficult to quickly review, but I checked your commit message and that was helpful as usual), this looks good at 103 commits. Let there not be a 104th. 😆

@namurphy

Copy link
Copy Markdown
Member Author

Thank you, @StanczakDominik! I am very ready for this to be merged. 😹 I am working on the summary commit message now.

A final thought is that some of the tests could likely be refactored into fixtures. I did not do this because I do not yet know how to write fixtures, but that is fine to leave for the future.

@StanczakDominik

Copy link
Copy Markdown
Member

That sounds like a possible future low priority issue. 😆

@namurphy
namurphy merged commit 7408cd9 into PlasmaPy:master Oct 31, 2018
@StanczakDominik

Copy link
Copy Markdown
Member

🎉

@namurphy
namurphy deleted the ionization-state branch October 31, 2018 22:21
@namurphy namurphy added the needs changelog entry See: https://docs.plasmapy.org/en/latest/contributing/changelog_guide.html label May 31, 2019
@V0lk3n V0lk3n mentioned this pull request Oct 7, 2019
@StanczakDominik StanczakDominik mentioned this pull request Jun 7, 2020
2 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs changelog entry See: https://docs.plasmapy.org/en/latest/contributing/changelog_guide.html plasmapy.particles Related to the plasmapy.particles subpackage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants