Skip to content

Change unit annotations to Quantity annotations - #2421

Merged
namurphy merged 32 commits into
PlasmaPy:mainfrom
namurphy:quantity-annotations
Dec 28, 2023
Merged

Change unit annotations to Quantity annotations#2421
namurphy merged 32 commits into
PlasmaPy:mainfrom
namurphy:quantity-annotations

Conversation

@namurphy

@namurphy namurphy commented Dec 20, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Description

This PR changes annotations like u.kg to u.Quantity[u.kg] so that they are true type hint annotations:

import astropy.units as u

# old style unit annotations
@validate_quantities
def magnetic_pressure(B: u.T) -> u.Pa:
    ...

# new style with actual type hint annotations
@validate_quantities
def magnetic_pressure(B: u.Quantity(u.T)) -> u.Quantity(u.Pa):
    ...

Motivation and context

At present, we use annotations like u.kg to specify the required units for @validate_quantities, which then verifies that the quantities are of the appropriate physical type and converts them to the correct units. However, this prevents us from using static type checking tools like mypy. While these are annotations, they are not type hint annotations because things like u.kg are not a type.

Enabling mypy (#2424) will let us catch errors in type hint annotations, which we currently have no way of doing.

Here's an example docstring before (with unit annotations) and after (with Quantity annotations).

Related issues

Related to: #268, #2424

@review-notebook-app

review-notebook-app Bot commented Dec 20, 2023

Copy link
Copy Markdown

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@github-actions github-actions Bot added docs PlasmaPy Docs at http://docs.plasmapy.org plasmapy.formulary Related to the plasmapy.formulary subpackage plasmapy.diagnostics Related to the plasmapy.diagnostics subpackage plasmapy.formulary.quantum Related to the plasmapy.formulary.quantum module plasmapy.dispersion Related to the plasmapy.dispersion subpackage notebooks Related to example Jupyter notebooks in docs/examples/ contributor guide labels Dec 20, 2023
@namurphy namurphy changed the title ', u.m**-3,' → ', u.Quantity[u.m**-3],' Change unit annotations to quantity annotations Dec 20, 2023
@codecov

codecov Bot commented Dec 20, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2ae8857) 96.91% compared to head (72353b5) 96.91%.
Report is 1 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #2421   +/-   ##
=======================================
  Coverage   96.91%   96.91%           
=======================================
  Files         104      104           
  Lines        9121     9121           
=======================================
  Hits         8840     8840           
  Misses        281      281

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions github-actions Bot added the plasmapy.utils Related to the plasmapy.utils subpackage label Dec 20, 2023
@github-actions github-actions Bot added plasmapy.particles Related to the plasmapy.particles subpackage plasmapy.simulation Related to the plasmapy.simulation subpackage labels Dec 20, 2023
@github-actions github-actions Bot added plasmapy.plasma Related to the plasmapy.plasma subpackage testing labels Dec 20, 2023
namurphy
namurphy commented Dec 20, 2023
View reviewed changes
Comment thread plasmapy/utils/decorators/checks.py Resolved
namurphy
namurphy commented Dec 20, 2023
View reviewed changes
Comment thread plasmapy/utils/decorators/checks.py Resolved
namurphy
namurphy commented Dec 20, 2023
View reviewed changes
Comment thread plasmapy/utils/decorators/converter.py Resolved
namurphy
namurphy commented Dec 20, 2023
View reviewed changes
Comment thread plasmapy/utils/decorators/validators.py Resolved
namurphy
namurphy commented Dec 20, 2023
View reviewed changes
Comment thread plasmapy/utils/decorators/validators.py Resolved
@namurphy namurphy mentioned this pull request Dec 21, 2023
Merged
@namurphy
namurphy marked this pull request as ready for review December 22, 2023 18:21
@namurphy namurphy changed the title Change unit annotations to quantity annotations Change unit annotations to Quantity annotations Dec 22, 2023
pheuer
pheuer approved these changes Dec 22, 2023
View reviewed changes

@pheuer pheuer 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.

Lots of changes, but looks pretty straightforward. I had to open a console to realize that u.Quantity[u.m] returns a typing annotation. The coding guide could I guess mention that, but the example is pretty clear.

Does this PR make this change on every instance in the package?

Comment thread pyproject.toml Outdated
"RUF013", # implicit-optional
"RUF015", # unnecessary-iterable-allocation-for-first-element
"RUF016", # invalid-index-type
"RUF018", # assignment-in-assert

@pheuer pheuer Dec 22, 2023

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.

Why remove this rule?

@namurphy namurphy Dec 28, 2023

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.

This is an experimental rule recently added by ruff, and I was locally getting warning messages about not running ruff with --preview enabled. It's unrelated to this PR.

@namurphy

namurphy commented Dec 28, 2023

Copy link
Copy Markdown
Member Author

Lots of changes, but looks pretty straightforward. I had to open a console to realize that u.Quantity[u.m] returns a typing annotation. The coding guide could I guess mention that, but the example is pretty clear.

Does this PR make this change on every instance in the package?

I used grep pretty heavily and I changed all instances of this I could find, but there might be some that are missing. Enabling mypy in #2424 will help us catch any remaining ones.

Thank you so much for the review!

@namurphy

namurphy commented Dec 28, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

Correction: this PR doesn't change all of the unit annotations across the package. In Alfven_speed and mass_density, there are parameters named density that accept Quantity objects with physical types of either number density or mass density. I'm not sure yet how best make this work with @validate_quantities, so I'll leave them for another PR.

@namurphy
namurphy enabled auto-merge (squash) December 28, 2023 19:30
@namurphy
namurphy merged commit 1c2d486 into PlasmaPy:main Dec 28, 2023
@namurphy
namurphy deleted the quantity-annotations branch December 28, 2023 19:32
@namurphy namurphy mentioned this pull request Jan 5, 2024
Merged
@namurphy namurphy mentioned this pull request Jan 17, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pheuer pheuer pheuer approved these changes
@ejohnson-96 ejohnson-96 Awaiting requested review from ejohnson-96
@rocco8773 rocco8773 Awaiting requested review from rocco8773 rocco8773 is a code owner

Assignees

No one assigned

Labels

contributor guide docs PlasmaPy Docs at http://docs.plasmapy.org notebooks Related to example Jupyter notebooks in docs/examples/ packaging Related to packaging or distribution plasmapy.diagnostics Related to the plasmapy.diagnostics subpackage plasmapy.dispersion Related to the plasmapy.dispersion subpackage 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 plasmapy.plasma Related to the plasmapy.plasma subpackage plasmapy.simulation Related to the plasmapy.simulation subpackage plasmapy.utils Related to the plasmapy.utils subpackage static type checking testing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@namurphy @pheuer