Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spin off thermal_gyroradius from gyroradius? #2540

Open
namurphy opened this issue Feb 27, 2024 · 2 comments
Open

Spin off thermal_gyroradius from gyroradius? #2540

namurphy opened this issue Feb 27, 2024 · 2 comments
Labels
breaking change For breaking changes, excluding deprecations and planned removals plasmapy.formulary Related to the plasmapy.formulary subpackage proposal Proposals that need to be decided upon before implementation status: needs discussion Issues & PRs that need to be discussed at a community meeting or by the Coordinating Committee
Milestone

Comments

@namurphy
Copy link
Member

namurphy commented Feb 27, 2024

Our gyroradius function is quite long and complex. The complex control flow can often lead to missed situations in tests. When a function is too long and complex, it's usually an indication that the function is doing too much, and that the function should be split up into multiple short functions that do exactly one thing.

The calculation of the gyroradius needs the perpendicular velocity, which can currently be given by providing the velocity itself, the Lorentz factor, or the temperature. A lot of the complexity arises because of validating inputs (like making sure that only one of the above is provided).

One way to reduce this complexity would be to create a separate function only for the thermal_gyroradius. I'm thinking that spelling out "thermal gyroradius" will make resulting code less ambiguous.

We could perhaps also add a keyword argument to thermal_gyroradius to specify whether it should be the "most probable", "rms", or other thermal velocity.

Before we implement this, it'd be helpful to get some feedback from a few people. Is this a worthwhile idea?

@namurphy namurphy added plasmapy.formulary Related to the plasmapy.formulary subpackage proposal Proposals that need to be decided upon before implementation breaking change For breaking changes, excluding deprecations and planned removals status: needs discussion Issues & PRs that need to be discussed at a community meeting or by the Coordinating Committee labels Feb 27, 2024
@namurphy namurphy added this to the v2024.5.0 milestone Feb 27, 2024
@jwreep
Copy link
Contributor

jwreep commented Mar 6, 2024

Would this issue be more easily solved with function overloading? In C++, at least, the trivial solution would be to have three functions with different inputs.

I haven't used it before, but from what I understand, overloading was added to Python > 3.8, so something like:

@overload
def gyroradius( lorentzfactor: u.dimensionless_unscaled ):
    ...
@overload
def gyroradius( T: u.K ):
    ...
def gyroradius( Vperp: u.cm/u.s ):
    ...

@namurphy
Copy link
Member Author

namurphy commented Mar 7, 2024

Interesting! It looks like the @overload operator is intended for static type checking, so we'd have to implement the overloading mechanics ourselves, which would be a lot less trivial than in C++ or Julia.

With that said, we recently started doing static type checking in PlasmaPy, and I think there are a few places where @overload would be quite helpful. Thank you for bringing this up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change For breaking changes, excluding deprecations and planned removals plasmapy.formulary Related to the plasmapy.formulary subpackage proposal Proposals that need to be decided upon before implementation status: needs discussion Issues & PRs that need to be discussed at a community meeting or by the Coordinating Committee
Projects
None yet
Development

No branches or pull requests

2 participants