Starting an attempt to remove default particle assumptions #911

arichar6 · 2020-10-05T20:41:47Z

This is my attempt to remove default values from functions that take a particle as input in formulary/parameters.py.

This is described in issue #453.

I have added a changelog entry for this pull request.

If adding new functionality, I have added tests and docstrings. (Well, mostly I changed the docstrings to reflect the fact that some arguments that used to be optional now are required.

I have fixed any newly failing tests.

…ormulary

rocco8773 · 2020-10-05T23:57:48Z

@arichar6 Thanks for taking this on! Issue #453 was created before I started working on plasmapy, but I absolutely agree with it. With that said, we shouldn't even give the ion and particle keywords are default value of None. Instead we should make them a required argument like...

def Alfven_speed(B, density, ion, z_mean=None):
    ...

or

def Alfven_speed(B, density, *, ion, z_mean=None):
    ...

The latter keeps ion as a keyword-only argument, whereas, the former makes it a keyword and positional argument.

Additionally, we should add a proper annotation Union[str, Particle], which would look something like...

def Alfven_speed(B: u.T, density: [u.m ** -3, u.kg / u.m ** 3], *, ion: Union[str, Particle], z_mean=None):
    ...

arichar6 · 2020-10-06T12:00:14Z

Thanks for the feedback!

I'll remove the None defaults, and add the type annotations.

Also added type annotations for the particles. Still need to fix the tests.

arichar6 · 2020-10-06T13:01:49Z

I still have to fix the tests that now fail. But the defaults should be fixed now, and type annotations added.

arichar6 · 2020-10-06T21:02:07Z

As I was working on the tests, I realized that one change might introduce a backwards-incompatibility.
I made the ion argument in the ion_sound_speed function a keyword-only argument. I'm not sure how to remove the default value without making it keyword-only, since arguments that come before it have default values.

An alternative would be to change the order of the arguments to the function, but that seems worse somehow.

Thoughts?

** edit **
Thinking about this more, I suspect it's ok. Removing the defaults is going to break backwards compatibility anyway. But I still would like to hear your thoughts on this.

codecov · 2020-10-06T21:22:45Z

Codecov Report

Merging #911 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #911      +/-   ##
==========================================
- Coverage   96.21%   96.21%   -0.01%     
==========================================
  Files          60       60              
  Lines        5417     5416       -1     
==========================================
- Hits         5212     5211       -1     
  Misses        205      205

Impacted Files	Coverage Δ
plasmapy/formulary/collisions.py	`97.19% <100.00%> (+0.43%)`	⬆️
plasmapy/formulary/parameters.py	`99.01% <100.00%> (+<0.01%)`	⬆️
plasmapy/utils/pytest_helpers/pytest_helpers.py	`93.04% <0.00%> (-0.37%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6d5651c...c1d3213. Read the comment docs.

namurphy

Thank you for working on this! Most of my suggestions are related to annotations for particle inputs, in particular that the annotation will need to be Particle for when we eventually implement the @particle_input decorator to this file (#341). Otherwise, all of the changes look great. Thank you again!

Many of the suggested changes are related to the @particle_input decorator which still needs to implemented in the formulary

Otherwise, all of the changes look great.

plasmapy/formulary/parameters.py

Co-authored-by: Nick Murphy <[email protected]>

plasmapy/formulary/parameters.py

arichar6 · 2020-10-08T12:29:11Z

One more question. Now that we are using Particle as the type annotation, the docstrings should probably be updated from saying something like ion : str to ion : Particle, correct?

rocco8773 · 2020-10-09T17:08:36Z

@arichar6 It appears the documentation build errors are due to the jupyter notebooks not being updated, specifically /docs/notebooks/physics.ipynb. Do you want to give fixing this a try? Just make sure before committing your changes that you clear all cell outputs.

rocco8773

This is looking good! I only have a few minor changes plus the update of the jupyter notebook.

The rest of the plasmapy.formulary needs this treatment too, but I'm ok keeping that to a separate/followup PR.

rocco8773 · 2020-10-09T17:20:05Z

plasmapy/formulary/parameters.py

+    *,
+    ion: Particle,


This should look like....

def ion_sound_speed( T_e: u.K, T_i: u.K, ion: Particle, n_e: u.m ** -3 = None, k: u.m ** -1 = None, gamma_e=1, gamma_i=3, z_mean=None, ) -> u.m / u.s:

*, overly simplified, indicates the end to the positional arguments, so keyword-only arguments should not be defined before it.

So you are ok with changing the order of the arguments in this function call? I was using the * entry so that I could keep the order of arguments the same, but have ion (with no default) come after the other arguments which do have defaults.

Keywords have no order associated with them, so doing this does not change the order at all. It does make ion a required argument that is either positional or keyword. So, the suggested makes the interface behave like...

ion_sound_speed(2*u.eV, 3*u.eV, "He+") # or ion_sound_speed(2*u.eV, 3*u.eV, ion="He+")

All the other arguments are keyword-only, so they can be specified in any order.

I'm not sure why you say that all other arguments are keyword-only. I just tried, and you can specify an electron density and k by just giving them as the next arguments. No keywords needed.

>>> ion_sound_speed(2*u.eV, 3*u.eV, "He+", 1 * u.m**-3, 1 * u.m ** -1) <Quantity 1.54899987 m / s>

plasmapy/formulary/parameters.py

changelog/911.breaking.rst

Co-authored-by: Erik Everson <[email protected]>

…char6/PlasmaPy into 453-remove-default-particles

arichar6 · 2020-10-09T20:30:53Z

Any insights into why the codecov test is failing now? I'm don't really quite know what it is check for, exactly.

rocco8773 · 2020-10-09T20:48:40Z

Any insights into why the codecov test is failing now? I'm don't really quite know what it is check for, exactly.

Looks like line 339 in collisions.py is not getting hit by tests. Without digging more, I'm not sure why this is the case.

arichar6 · 2020-10-13T14:04:05Z

Looks like line 339 in collisions.py is not getting hit by tests. Without digging more, I'm not sure why this is the case.

I might be wrong, but it looks like line 339 in collisions.py is not accessible. Here are the preceding lines:

elif np.any(np.isnan(V)):
    if np.isscalar(V.value) and np.isscalar(T.value):
        V = parameters.thermal_speed(T, "e-", mass=m)
    elif np.isscalar(V.value):
        V = parameters.thermal_speed(T, "e-", mass=m)

I just tried this, and if np.any(np.isnan(V)) returns True for a numpy array (with astropy units), then np.isscalar(V.value) returns False. Was there some other case that this was trying to test for?

namurphy · 2020-10-13T16:35:25Z

I might be wrong, but it looks like line 339 in collisions.py is not accessible. Here are the preceding lines:

I just tried this, and if np.any(np.isnan(V)) returns True for a numpy array (with astropy units), then np.isscalar(V.value) returns False. Was there some other case that this was trying to test for?

Hm...that's a good question. That's a bit strange so it would be worth looking into it further. It does seem outside of the intended scope of this pull request, so my suggestion would be to create a brief issue on it so that we can address it in a different pull request. I'm not a fan of long extensive if/elif statements so I'm wondering if there's a way to replace these if/elif blocks with polymorphism...but that is definitely for another day.

namurphy · 2020-10-13T16:50:13Z

To follow up: there are a few other things in collisions.py that should probably be cleaned up, so I can create the other issue.

changelog/911.breaking.rst

plasmapy/formulary/parameters.py

plasmapy/formulary/collisions.py

Co-authored-by: Erik Everson <[email protected]>

…char6/PlasmaPy into 453-remove-default-particles

arichar6 · 2020-10-14T12:58:55Z

Ok. I think this PR is finally done. While this does not complete the changes required for #453, it does complete the changes needed in parameters.py.

rocco8773

Looks good to me! Thanks making these improvement.

namurphy

Thank you again for doing this! It looks good to me so I think it's ready to merge.

Steve Richardson added 3 commits October 5, 2020 16:35

Starting an attempt to remove default particle assumptions from the f…

263499c

…ormulary

Changing defaults to None

452ad03

fixing tests and text in docstrings

ede9521

rocco8773 added refactoring ♻️ Improving an implementation without adding new functionality plasmapy.formulary Related to the plasmapy.formulary subpackage labels Oct 5, 2020

rocco8773 requested review from rocco8773, StanczakDominik and namurphy October 5, 2020 23:57

removed default values of None

c4a429f

Also added type annotations for the particles. Still need to fix the tests.

Got the tests to pass

46a1ea0

namurphy requested changes Oct 8, 2020

View reviewed changes

Steve Richardson and others added 9 commits October 8, 2020 08:04

Annotate with Particle instead of Union[str, Particle]

382a666

Co-authored-by: Nick Murphy <[email protected]>

Annotate with Particle instead of Union[str, Particle]

d64660c

Co-authored-by: Nick Murphy <[email protected]>

Annotate with Particle instead of Union[str, Particle]

41c8d9f

Co-authored-by: Nick Murphy <[email protected]>

Annotate with Particle instead of Union[str, Particle]

4dc75c1

Co-authored-by: Nick Murphy <[email protected]>

Annotate with Particle instead of Union[str, Particle]

d6ae8dd

Co-authored-by: Nick Murphy <[email protected]>

Annotate with Particle instead of Union[str, Particle]

55a071c

Co-authored-by: Nick Murphy <[email protected]>

Annotate with Particle instead of Union[str, Particle]

8476275

Co-authored-by: Nick Murphy <[email protected]>

Make ion argument optional

2ec3d39

caught one more Union annotation

f23b5c4

arichar6 commented Oct 8, 2020

View reviewed changes

plasmapy/formulary/parameters.py Show resolved Hide resolved

quick bug fix

52976d6

arichar6 marked this pull request as ready for review October 9, 2020 16:48

Steve Richardson added 2 commits October 9, 2020 12:54

reformat code to conform to style guid

cf16017

one more style fix

693f701

updated jupyter notebook in the documentation

430e756

rocco8773 requested changes Oct 9, 2020

View reviewed changes

Steve Richardson and others added 5 commits October 9, 2020 14:09

Update changelog/911.breaking.rst

149baf3

Co-authored-by: Erik Everson <[email protected]>

update ion_sound_speed function

a89ee12

Merge branch '453-remove-default-particles' of https://github.com/ari…

238e210

…char6/PlasmaPy into 453-remove-default-particles

reformatted file to conform to style

2e29f03

another try to get the style right

8f16e36

namurphy mentioned this pull request Oct 13, 2020

Refactor collisions.py #931

Open

7 tasks

rocco8773 requested changes Oct 13, 2020

View reviewed changes

changelog/911.breaking.rst Outdated Show resolved Hide resolved

plasmapy/formulary/parameters.py Outdated Show resolved Hide resolved

rocco8773 reviewed Oct 13, 2020

View reviewed changes

plasmapy/formulary/collisions.py Outdated Show resolved Hide resolved

Steve Richardson and others added 3 commits October 14, 2020 08:21

Update plasmapy/formulary/parameters.py

15f74fe

Co-authored-by: Erik Everson <[email protected]>

fix issue in collisions.py to improve coverage

6bd19fe

Merge branch '453-remove-default-particles' of https://github.com/ari…

c1d3213

…char6/PlasmaPy into 453-remove-default-particles

rocco8773 approved these changes Oct 14, 2020

View reviewed changes

namurphy approved these changes Oct 14, 2020

View reviewed changes

rocco8773 merged commit 3ecf372 into PlasmaPy:master Oct 14, 2020

arichar6 deleted the 453-remove-default-particles branch October 14, 2020 18:02

rocco8773 mentioned this pull request Oct 14, 2020

Improve Hall_parameter signature and docstring #915

Closed

rocco8773 mentioned this pull request Jan 14, 2021

Create plasmapy.dispersion.dispersionsolver & associated functionality #932

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Starting an attempt to remove default particle assumptions #911

Starting an attempt to remove default particle assumptions #911

arichar6 commented Oct 5, 2020 •

edited

Loading

rocco8773 commented Oct 5, 2020

arichar6 commented Oct 6, 2020

arichar6 commented Oct 6, 2020

arichar6 commented Oct 6, 2020 •

edited

Loading

codecov bot commented Oct 6, 2020 •

edited

Loading

namurphy left a comment

arichar6 commented Oct 8, 2020

rocco8773 commented Oct 9, 2020

rocco8773 left a comment

rocco8773 Oct 9, 2020

arichar6 Oct 9, 2020

rocco8773 Oct 9, 2020

arichar6 Oct 9, 2020

arichar6 commented Oct 9, 2020

rocco8773 commented Oct 9, 2020

arichar6 commented Oct 13, 2020 •

edited

Loading

namurphy commented Oct 13, 2020

namurphy commented Oct 13, 2020

arichar6 commented Oct 14, 2020

rocco8773 left a comment

namurphy left a comment

Starting an attempt to remove default particle assumptions #911

Starting an attempt to remove default particle assumptions #911

Conversation

arichar6 commented Oct 5, 2020 • edited Loading

rocco8773 commented Oct 5, 2020

arichar6 commented Oct 6, 2020

arichar6 commented Oct 6, 2020

arichar6 commented Oct 6, 2020 • edited Loading

codecov bot commented Oct 6, 2020 • edited Loading

Codecov Report

namurphy left a comment

Choose a reason for hiding this comment

arichar6 commented Oct 8, 2020

rocco8773 commented Oct 9, 2020

rocco8773 left a comment

Choose a reason for hiding this comment

rocco8773 Oct 9, 2020

Choose a reason for hiding this comment

arichar6 Oct 9, 2020

Choose a reason for hiding this comment

rocco8773 Oct 9, 2020

Choose a reason for hiding this comment

arichar6 Oct 9, 2020

Choose a reason for hiding this comment

arichar6 commented Oct 9, 2020

rocco8773 commented Oct 9, 2020

arichar6 commented Oct 13, 2020 • edited Loading

namurphy commented Oct 13, 2020

namurphy commented Oct 13, 2020

arichar6 commented Oct 14, 2020

rocco8773 left a comment

Choose a reason for hiding this comment

namurphy left a comment

Choose a reason for hiding this comment

arichar6 commented Oct 5, 2020 •

edited

Loading

arichar6 commented Oct 6, 2020 •

edited

Loading

codecov bot commented Oct 6, 2020 •

edited

Loading

arichar6 commented Oct 13, 2020 •

edited

Loading