change behaviour of gyroradius when arguments are nan #1187
Conversation
github-actions
bot
added
the
plasmapy.formulary
Codecov Report
@@ Coverage Diff @@
## main #1187 +/- ##
=======================================
Coverage ? 96.98%
=======================================
Files ? 71
Lines ? 7022
Branches ? 0
=======================================
Hits ? 6810
Misses ? 212
Partials ? 0 Continue to review full report at Codecov.
|
| @@ -1198,7 +1198,7 @@ def gyroradius( | |||
|
|
|||
| # check 1: ensure either Vperp or T_i invalid, keeping in mind that | |||
| # the underlying values of the astropy quantity may be numpy arrays | |||
| if np.any(np.logical_not(np.logical_xor(isfinite_Vperp, isfinite_Ti))): | |||
| if np.any(np.logical_and(isfinite_Vperp, isfinite_Ti)): | |||
There was a problem hiding this comment.
Do you think we could test this to ensure it works as expected? The tests for gyroradius are located around https://github.com/PlasmaPy/PlasmaPy/blob/main/plasmapy/formulary/tests/test_parameters.py#L798 here :)
There was a problem hiding this comment.
Ahh, didn't knew about that proper test section, sorry - had only made tests on my own. Will do the proper testing later.
There was a problem hiding this comment.
Does testing here simply means to run test_gyroradius(), or also to modify test_gyroradius() and add some new tests? (The former one should be sufficient in my opinion but I might overlook something)
There was a problem hiding this comment.
The tests are ran automatically anyway (see the Checks tab!), so that's already being done :) What I meant instead was to add (or ensure there are already! haven't checked 😅) a few checks to test_gyroradius:
- gyroradius(T_i = nan) == nan, no exception;
- gyroradius(Vperp = nan) == nan, no exception;
- gyroradius(T_i = nan, Vperp = nan) raises exception (there should be a
pytest.raisesclause around that general area we could adapt :) )
(np.isnan would be helpful for the comparison, since anything == np.nan is False, even if that anything is np.nan!)
There was a problem hiding this comment.
Those are exactly the checks I have done manually :)
Now I will try to understand the syntax in the test_gyroradius function and include them there.
There was a problem hiding this comment.
Note that this might not be (well, I'm pretty sure it is not) the most elegant or "pythonic" way of testing it, but all the three new cases are included via assert tests. Not sure though if this qualifies the official test criteria....?
Happy to learn how to improve the tests, if desired.
Also: thanks for your patience :)
There was a problem hiding this comment.
This is literally what I'm here for :)
They're great, exactly what I was hoping for! Since the test suite isn't exploding, we can probably merge it now :)
|
@StanczakDominik I don't understand why the
I have no idea what was actually reformatted, so I don't know what to change to let this check pass... Any hints are greatly appreciated |
|
Iirc the magic incantation for this is... pre-commit.ci autofix |
for more information, see https://pre-commit.ci
|
No idea about magic incantation, sorry :D |
|
Hah, sorry! This part is a bit new, and we need to document it better. |
|
Awesome :) |
|
Yaayyy! Thank you! |
This PR closes #1022 . The general behaviour of
gyroradiusis changed such that if one of the arguments (T_i,Vperp) is nan, and the other is not supplied, no error is raised, but nan is returned.docstrings.