Visitar URL original
Expand usage of `particle_input` in `plasmapy.formulary` by namurphy · Pull Request #1732 · PlasmaPy/PlasmaPy · GitHub
Skip to content

Expand usage of particle_input in plasmapy.formulary - #1732

Merged
namurphy merged 23 commits into
PlasmaPy:mainfrom
namurphy:particle-input-formulary-1
Sep 15, 2022
Merged

namurphy merged 23 commits into
PlasmaPy:mainfrom
namurphy:particle-input-formulary-1

Conversation

@namurphy

@namurphy namurphy commented Sep 8, 2022 •

Copy link
Copy Markdown
Member

This PR is to use @particle_input in more of the functions in plasmapy.formulary that accept particles as inputs. This follows the particle_input refactoring in #1057, and complements #1718 (which updates docstrings).

I'm planning on additional pull requests that follow up on #1057, so I'm not planning on addressing everything here. In particular, this PR won't address collisions.py or braginskii.py.

@github-actions github-actions Bot added plasmapy.formulary Related to the plasmapy.formulary subpackage plasmapy.formulary.quantum Related to the plasmapy.formulary.quantum module labels Sep 8, 2022
@github-actions

Copy link
Copy Markdown

Thank you for contributing to PlasmaPy! The project's future depends deeply on contributors like you, so we deeply appreciate it! 🌱 The following checklist will be used by the code reviewer to help guide the code review process.

  • Overall
    • Does the PR do what it intends to do?
    • Except for very minor changes, is a changelog entry included and consistent with the changelog guide?
    • Are the continuous integration checks passing? (Most linter problems can be automagically fixed by commenting on this PR with pre-commit.ci autofix.)
  • Code
    • Is new/updated code understandable and consistent with the coding guide?
    • Are there ways to greatly simplify the implementation?
    • Are there any large functions that should be split up into shorter functions?
  • Tests
    • Are tests added/updated as required, and consistent with the testing guide?
    • Are the tests understandable?
    • Do the tests cover all important cases?
  • Docs
    • Are docs added/updated as required, and consistent with the doc guide?
    • Are the docs understandable?
    • Do the docs show up correctly in the preview, including Jupyter notebooks?

@github-actions github-actions Bot added the plasmapy.particles Related to the plasmapy.particles subpackage label Sep 14, 2022
@codecov

codecov Bot commented Sep 14, 2022 •

Copy link
Copy Markdown

Codecov Report

Base: 98.12% // Head: 98.13% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (a2108fb) compared to base (19075c9).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1732   +/-   ##
=======================================
  Coverage   98.12%   98.13%           
=======================================
  Files          87       87           
  Lines        8125     8134    +9     
=======================================
+ Hits         7973     7982    +9     
  Misses        152      152           
Impacted Files Coverage Δ
plasmapy/particles/decorators.py 99.41% <ø> (ø)
plasmapy/formulary/dimensionless.py 100.00% <100.00%> (ø)
plasmapy/formulary/distribution.py 100.00% <100.00%> (ø)
plasmapy/formulary/lengths.py 100.00% <100.00%> (ø)
plasmapy/formulary/quantum.py 100.00% <100.00%> (ø)
plasmapy/particles/_factory.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@namurphy
namurphy marked this pull request as ready for review September 14, 2022 21:02
@namurphy
namurphy requested a review from a team as a code owner September 14, 2022 21:02
@namurphy
namurphy requested review from StanczakDominik and removed request for a team September 14, 2022 21:02

@StanczakDominik StanczakDominik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good in general! One question - isn't there anything we should be worrying about from a deprecation standpoint here? Are all the functions backwards compatible?

Comment thread changelog/1732.trivial.1.rst Resolved
vx_drift=0,
vy_drift=0,
vTh=np.nan,
units="units",

@StanczakDominik StanczakDominik Sep 15, 2022 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to do a scroll-up-double-take at what the units argument meant here. Just now realizing that it might be too much customization and we should probably validate the units initially (O(1)), force unitless and append them at the end. Obviously out of scope for this PR, but I'll make an issue out of this.

Oooooh, nevermind, that's not what that does... I'll have to think about it some more.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if the parameter should be skip_validations instead of units. In any case, that's for another day!



# TODO: Use @check_relativistic and @particle_input
# TODO: Use @check_relativistic

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just chippin' away 😆 always love to see progress like this!

Co-authored-by: Dominik Stańczak-Marikin <stanczakdominik@gmail.com>
@namurphy

Copy link
Copy Markdown
Member Author

Looks good in general! One question - isn't there anything we should be worrying about from a deprecation standpoint here? Are all the functions backwards compatible?

That is indeed an important question! Overall this would be an expansion of functionality so that CustomParticle and ParticleList instances should now be able to work with most of these functions (though we still very much need to add tests to make sure that there are no incompatibilities). There was one difference in the exception being raised in test_quantum.py but otherwise there weren't any substantive changes to tests. It's still possible that there will be different exceptions raised for some cases, though those would be limited to probably rare cases that we're not testing.

@namurphy

Copy link
Copy Markdown
Member Author

Oh, and thank you for the review!

@namurphy
namurphy enabled auto-merge (squash) September 15, 2022 17:41
@namurphy
namurphy merged commit f0d1698 into PlasmaPy:main Sep 15, 2022
namurphy added a commit to namurphy/PlasmaPy that referenced this pull request Oct 25, 2022
@namurphy
namurphy deleted the particle-input-formulary-1 branch December 21, 2022 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

plasmapy.formulary.quantum Related to the plasmapy.formulary.quantum module plasmapy.formulary Related to the plasmapy.formulary subpackage plasmapy.particles Related to the plasmapy.particles subpackage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants