Generalize ion sound speed #700
Conversation
Addresses issue #316. Generalizes ion_sound_speed to work for any case, not just for the limit when (k * lambda_D) ** 2 is small. Additional term from Chapter 5.1, Froula, Glenzer, Luhmann and Sheffield "Plasma Scattering of Electromagnetic Radiation". Docstring updated to remove note about non-dispersive limit. Updated docstring examples, and included new example with a large value for (k * lambda_D) ** 2. Updated tests for ion_sound_speed, and added additional tests for the (general) large limit. Updated the assert_can_handle_nparray pytest helper method to work for the additional parameter, "k", added to the ion_sound_speed function.
|
Hello @tvarnish! Thanks for updating your pull request.
Comment last updated at 2019-10-20 09:53:16 UTC |
|
(I haven't added a changelog entry to the PR yet as I wasn't sure what type to use.) |
Codecov Report
@@ Coverage Diff @@
## master #700 +/- ##
==========================================
- Coverage 95.05% 95.02% -0.04%
==========================================
Files 57 57
Lines 4711 4722 +11
==========================================
+ Hits 4478 4487 +9
- Misses 233 235 +2
Continue to review full report at Codecov.
|
It's definitely a |
|
I just merged #692 in, so this PR will also need to merge those changes, since the location for |
Specify default values for k and n_e such that if values are not specified by the user, assume the non-dispersive limit for ion_sound_speed.
|
@StanczakDominik I'm still getting the hang of using GitHub, so I just wanted to check (before I break anything): how should I go about merging that PR into this one? Should I pull the changes into my local master, then merge that with my local branch? |
|
Precisely! I would thus go: git commit # your current changes if any
git checkout master
git pull # assuming you have `master` set up to track PlasmaPy/PlasmaPy#master, not tvarnish/PlasmaPy#master
git checkout generalize_ion_sound_speed
git merge master |
|
Great! I think that's worked. |
|
You'll still need to push the merge commit, though! :) |
There was a problem hiding this comment.
I'd set default values to None and handle None accordingly in the code. Plus, a None default values is more explicit to what is actually happening...the user did not specify a value.
I did not add it in the code review, but you could also issue a UserWarning of some kind if the user only passes in k or n_e, and not both.
Change default values of k and n_e to None, and add checks to ensure the user has given both values. Assume non-dispersive limit if user has only given one of k or n_e. Add tests to check that a UserWarning is thrown if only one of k or n_e has been given.
|
Thanks for the feedback @rocco8773! Using |
|
Thanks @rocco8773 for handling this! I wasn't able to look at it yesterday :( |
|
And the warning message is awesome! :) |
Fix warning when only given one of (k, n_e). Change Userwarning to PhysicsWarning.
plasmapy/formulary/parameters.py
Outdated
| if n_e is None or k is None: | ||
| klD2 = 0.0 | ||
| else: | ||
| lambda_D = Debye_length(T_e, n_e) | ||
| klD2 = (k * lambda_D) ** 2 |
There was a problem hiding this comment.
As the above comment suggests, place warning in this if-else statement
if n_e is None or k is None:
klD2 = 0.0
if n_e is None and k is None:
warnings.warn("say stuff")
else:
lambda_D = Debye_length(T_e, n_e)
klD2 = (k * lambda_D) ** 2There was a problem hiding this comment.
Do you think the warning should just go within the first if statement (rather than being within the nested if statement)? That way, the function would warn the user whenever the non-dispersive limit is assumed.
There was a problem hiding this comment.
Or... do you think it would make sense to only call the warning when one of k or n_e is given, so scripts that are dependant on PlasmaPy and were previously happy to use the non-dispersive limit don't cause warnings to be shown?
There was a problem hiding this comment.
Good point. I think the n_e=None and k=None case should not throw a warning since it is the default state. @StanczakDominik correct me if you think otherwise. In that case, I would restructure it like...
klD2 = 0.0
if (n_e is None) ^ (k is None):
warnings.warn("say stuff")
elif n_e is not None and k is not None:
lambda_D = Deby_length(T_e, n_e)
klD2 = (k * lambda_D) ** 2The parenthesis are needed in this case since the xor operator ^ only acts on booleans. The parenthesis force the conditionals into booleans before the xor operation is poerformed.
There was a problem hiding this comment.
Neat! I never knew there was an XOR operator in python. I see what you’re saying about the default case. That makes sense.
There was a problem hiding this comment.
Yeah, defaults should probably not throw warnings! I haven't (consciously) seen XOR in Python yet either. I like the proposed way of doing things :)
There was a problem hiding this comment.
Aaaaand that's lovely! Let's just wait for the tests to pass and that's a merge in my eyes! :)
Thanks @tvarnish !
|
And thanks @rocco8773 for the very helpful advice! |
|
Brilliant! I'm pleased I could help. :) Thank you @StanczakDominik and @rocco8773 for your really helpful code reviews and advice! |
|
@StanczakDominik I think the |
* Generalize ion_sound_speed function Addresses issue #316. Generalizes ion_sound_speed to work for any case, not just for the limit when (k * lambda_D) ** 2 is small. Additional term from Chapter 5.1, Froula, Glenzer, Luhmann and Sheffield "Plasma Scattering of Electromagnetic Radiation". Docstring updated to remove note about non-dispersive limit. Updated docstring examples, and included new example with a large value for (k * lambda_D) ** 2. Updated tests for ion_sound_speed, and added additional tests for the (general) large limit. Updated the assert_can_handle_nparray pytest helper method to work for the additional parameter, "k", added to the ion_sound_speed function. * Update contributors list * Fix ion_sound_speed docstring examples * Remove duplicate parameters in examples * Add changelog entry (#700) * Change k and n_e to optional parameters Specify default values for k and n_e such that if values are not specified by the user, assume the non-dispersive limit for ion_sound_speed. * Fix default values for k and n_e Change default values of k and n_e to None, and add checks to ensure the user has given both values. Assume non-dispersive limit if user has only given one of k or n_e. Add tests to check that a UserWarning is thrown if only one of k or n_e has been given. * Revert some tests back to default values (k, n_e) * Change UserWarning to PhysicsWarning Fix warning when only given one of (k, n_e). Change Userwarning to PhysicsWarning. * Restructure PhysicsWarning (for k and n_e)
|
Insert expletive here. Curse the middle click on Linux! Thanks a ton for pointing that out; I've cleaned it up on the master branch. Phew. And yeah, that was all me. 😆 |
rocco8773
added
the
plasmapy.formulary
Closes #316. Generalizes
ion_sound_speedto work for any case, not just for the limit when(k * lambda_D) ** 2is small. Additional term from Chapter 5.1, Froula, Glenzer, Luhmann and Sheffield "Plasma Scattering of Electromagnetic Radiation".Docstring updated to remove note about the non-dispersive limit. Updated docstring examples, and included a new example with a large value for
(k * lambda_D) ** 2.Updated tests for ion_sound_speed, and added additional tests for the (general) large limit. Updated the
assert_can_handle_nparraypytest helper method to work for the additional parameter,k,added to
ion_sound_speed.