Update parameters.thermal_speed #850
Conversation
|
Hello @pheuer! Thanks for updating your pull request. Congratulations! There are no PEP8 issues in this pull request. 😸 Comment last updated at 2020-06-09 20:36:58 UTC |
Codecov Report
@@ Coverage Diff @@
## master #850 +/- ##
=======================================
Coverage 95.94% 95.94%
=======================================
Files 56 56
Lines 5126 5128 +2
=======================================
+ Hits 4918 4920 +2
Misses 208 208
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
The changes and the notebook look great!
- I'm getting some warnings from doc build (see CircleCI), which my suggestions might address
- I see a possible simplifying refactor for the function without resorting to the Gamma function
- and the newly added style checks are complaining. You should be able to apply style linting automatically by using
pip install pre-commit
cd $PLASMAPY_CLONED_REPO
pre-commit install
pre-commit run --all-filesIt's freshly refreshed functionality, so I'd appreciate feedback on how that ends up working! 🙂
Co-authored-by: Dominik Stańczak <[email protected]>
Co-authored-by: Dominik Stańczak <[email protected]>
…/PlasmaPy into update_thermal_speed_#427
…eed_#427 Merging with master
|
@StanczakDominik Thanks for the suggestions! I've implemented the dictionary refactor: that's very elegant. A few other changes:
|
I don't think there is, or at least I'm not aware of it yet :( I'll give this a quick once-over again and merge it, most likely :) |
Co-authored-by: Dominik Stańczak <[email protected]>
Co-authored-by: Dominik Stańczak <[email protected]>
Co-authored-by: Dominik Stańczak <[email protected]>
|
@StanczakDominik Looks good - most of the tests I've written for both this and the Thomson module have been known value comparisons: I can see the potential problems with this approach but I can't think of a better way to ensure the physics equations haven't been modified? |
Me neither, unless the original derivation provided value samples... Which they usually don't. Oh well. |
|
We could potentially try property based testing one of these days... I'd have to pick it up from scratch though. No matter! Merging :) |
rocco8773
added
the
plasmapy.formulary
Scrolling through the issue log, I saw #427 and thought it was an easy addition. However, while making the example I thought that since 1D, 2D, and 3D Maxwellians are included in the distribution.py module, thermal speeds for different dimensionalities should also be supported (if for no other reason than to remind people to use the right one). Does everyone agree this is worthwhile functionality?
In this PR I've added this functionality and wrote appropriate tests. I also created a thermal_speed example notebook that contains the plot originally requested in #427 and have added this example to the documentation.
Closes #427