Convert from energy to velocity in relativistic formulary (#1056) #1297
Conversation
github-actions
bot
added
the
plasmapy.formulary
There was a problem hiding this comment.
This is a good start! In addition to the comments I left, you will also need to write test functions that test this new function. For examples, look in the file formulary/tests/test_relativity.py. Any function in these files with a name starting in 'test" will be automatically run by the unit test module every time a new change to plasmapy is made, to make sure this function is still performing as expected.
(You may already know this, but you can make changes to this PR by pushing new commits to your branch. In the future, it's usually better to make a new branch for a PR rather than using your main branch (Tlord18:main). But, this should be ok for now.)
| value: [u.J, u.kg * u.m / u.s], | ||
| particle: ParticleLike, | ||
| is_kinetic_energy: bool = False, | ||
| ) -> u.m / u.s: |
There was a problem hiding this comment.
Add a docstring. Look at the other formulary functions to see how to format it, and what kind of information should be included. Make sure to explicitly explain the three separate ways this can be used
- Momentum -> velocity
- KE -> velocity
- Total energy -> velocity
| particle: ParticleLike, | ||
| is_kinetic_energy: bool = False, | ||
| ) -> u.m / u.s: | ||
| if value.units.is_equivalent(u.J): |
There was a problem hiding this comment.
This needs an "else" that includes the p=value line. Right now, no matter what happens here, is overwritten by value.
| - ((particle.mass ** 2) * (c ** 4)) | ||
| ) | ||
| else: | ||
| p = (1 / c) * sqrt((value ** 2) - ((particle.mass ** 2) * (c ** 4))) |
There was a problem hiding this comment.
Better to use np.sqrt everywhere, rather than sqrt?
There was a problem hiding this comment.
Thank you for making this contribution! This will be an important thing to have in plasmapy.formulary. A few thoughts...
-
It'd also be helpful to use the
plasmapy.particles.particle_inputdecorator here, which auto-converts a representation of a particle into aParticle. (The annotation would have to be changed fromParticleLiketoParticlefor the time being, but that will change in the refactoring ofparticle_inputthat I'm hoping to get to by our 0.8 release.) -
We just updated our doc guide and testing guide which might be helpful for some of @pheuer's comments.
Thank you again!
| is_kinetic_energy: bool = False, | ||
| ) -> u.m / u.s: | ||
| if value.units.is_equivalent(u.J): | ||
| if is_kinetic_energy: |
There was a problem hiding this comment.
I'm wondering if using the physical_type attribute of units would make it a little more readable. Either way would be fine with me, though.
if value.unit.physical_type == "energy":
|
Thanks a lot for these comments, I'll implement them and commit them soon. I must've forgotten the else, so I'll change that. I'll also get started on the tests and the documentation
Ah yes, I had initially created a branch, where I had made a few sample commits initially. In an attempt to squash them before making a PR, I kinda messed it up somewhere. So I ended up making a PR through the main branch. I'll make a new branch again. Thanks a lot! |
docstrings.