Repository navigation
Create classes to represent ionization state distributions - #404
Conversation
|
Hello @namurphy! Thanks for updating your pull request.
Comment last updated on August 07, 2018 at 18:59 Hours UTC |
lemmatum
left a comment
There was a problem hiding this comment.
Looks like a good start! Needs some more basic functionality prior to merge.
02e758d to
749a6bd
Compare
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.
a78cc8f to
097f899
Compare
3a79b10 to
aabc090
Compare
- 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.
|
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. |
|
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? |
|
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
left a comment
There was a problem hiding this comment.
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. 😆
|
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. |
|
That sounds like a possible future low priority issue. 😆 |
|
🎉 |
IonizationState and IonizationStateCollection
#579
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.0release to merge this, since this PR is only for a partial implementation anyway.