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

Make unit validators compatible with u.Quantity[u.T]-style annotations #1661

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

StanczakDominik
Copy link
Member

@StanczakDominik StanczakDominik commented Aug 4, 2022

Possibly closes #1413 and also closes #1414, maybe? Let's see if this blows up.

Essentially what happens when you do, say, def f(B: u.Quantity[u.T]) is that the annotation becomes a typing._AnnotatedAlias. Apparently the canonical way of reading type hints for a function is typing.get_type_hints, but since we didn't go that route for the existing code, I didn't want to start rewriting the whole thing just yet. Instead, the annotation has a __metadata__ field, an iterable of things within the [] parenthesis. So I just picked the first item in that for now.

I've also added this kind of annotation in frequencies for now, to see whether they'd explode.

  • I have added a changelog entry for this pull request.
  • If adding new functionality, I have added tests and
    docstrings.
  • I have fixed any newly failing tests.

@github-actions github-actions bot added plasmapy.formulary Related to the plasmapy.formulary subpackage plasmapy.utils Related to the plasmapy.utils subpackage labels Aug 4, 2022
@StanczakDominik
Copy link
Member Author

StanczakDominik commented Aug 4, 2022

hey @rocco8773, I wanted to ping you for short feedback on what you think about this approach :)

@StanczakDominik
Copy link
Member Author

I wonder whether we shouldn't deprecate the old, u.m style syntax.

@codecov
Copy link

codecov bot commented Aug 4, 2022

Codecov Report

Base: 98.31% // Head: 97.19% // Decreases project coverage by -1.11% ⚠️

Coverage data is based on head (71371ff) compared to base (5e542ce).
Patch coverage: 75.00% of modified lines in pull request are covered.

❗ Current head 71371ff differs from pull request most recent head d65ae60. Consider uploading reports for the commit d65ae60 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1661      +/-   ##
==========================================
- Coverage   98.31%   97.19%   -1.12%     
==========================================
  Files          95       84      -11     
  Lines        8404     8020     -384     
==========================================
- Hits         8262     7795     -467     
- Misses        142      225      +83     
Impacted Files Coverage Δ
plasmapy/utils/decorators/checks.py 99.12% <25.00%> (-0.58%) ⬇️
plasmapy/formulary/frequencies.py 100.00% <100.00%> (ø)
plasmapy/formulary/ionization.py 100.00% <100.00%> (ø)
plasmapy/formulary/lengths.py 100.00% <100.00%> (ø)
plasmapy/__init__.py 33.33% <0.00%> (-66.67%) ⬇️
plasmapy/formulary/quantum.py 68.67% <0.00%> (-31.33%) ⬇️
plasmapy/simulation/abstractions.py 76.04% <0.00%> (-23.96%) ⬇️
plasmapy/plasma/plasma_base.py 75.60% <0.00%> (-5.16%) ⬇️
plasmapy/analysis/fit_functions.py 98.55% <0.00%> (-1.45%) ⬇️
plasmapy/plasma/sources/openpmd_hdf5.py 81.52% <0.00%> (-0.90%) ⬇️
... and 58 more

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.

@StanczakDominik
Copy link
Member Author

StanczakDominik commented Aug 4, 2022

Oh yeah, here's the crux of the issue. typing._AnnotatedAlias, which is somehow related to typing.Annotated as I understand it, requires python 3.9. Without it, we can't even begin to annotate the functions themselves.

@StanczakDominik
Copy link
Member Author

StanczakDominik commented Aug 4, 2022

@StanczakDominik
Copy link
Member Author

StanczakDominik commented Aug 23, 2022

@StanczakDominik StanczakDominik self-assigned this Oct 8, 2022
@github-actions
Copy link

github-actions bot commented Oct 9, 2022

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?

@StanczakDominik
Copy link
Member Author

StanczakDominik commented Oct 9, 2022

Looks like this won't work without a proper rewrite of check_units to use the functionality from typing :(

@namurphy namurphy added priority: high Issues & PRs with significant urgency and importance that should be addressed soon Python Lv3 | Proficient Issues that require proficiency in Python Plasma Lv1 | Beginner Issues appropriate for someone who has some knowledge of physics status: dormant PRs that are stalled and removed priority: high Issues & PRs with significant urgency and importance that should be addressed soon Python Lv3 | Proficient Issues that require proficiency in Python Plasma Lv1 | Beginner Issues appropriate for someone who has some knowledge of physics labels May 26, 2023
@namurphy namurphy added the size: medium 30 ≤ changed lines < 100 label May 26, 2023
@namurphy namurphy added this to the Future milestone Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plasmapy.formulary Related to the plasmapy.formulary subpackage plasmapy.utils Related to the plasmapy.utils subpackage size: medium 30 ≤ changed lines < 100 status: dormant PRs that are stalled
Projects
None yet
2 participants