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

update converters for pint 0.22 #880

Merged
merged 8 commits into from
Jun 13, 2023

Conversation

braingram
Copy link
Contributor

@braingram braingram commented Jun 2, 2023

in pint 0.22 Quantity appears at pint.registry.Quantity and requires an update to the corresponding asdf
Converter (the same for pint.registry.Unit).

Changes

I opted to add the type pint.Quantity to the types supported by PintQuantityConverter. Types and/or strings can be used here and both have pros/cons. Using the type requires that pint be imported when the Converter is created (which can slow down initialization while asdf loads extensions. However this doesn't appear to be an issue here as pint is already imported when weldx.tags.units.pint_quantity is imported. By using the type, pint is free to move the internal implementation of Quantity (which can change the class path) but as long as it appears importable via pint.Quanity the Converter should stay up-to-date.

In this case the string "pint.Quantity" also needs to be included as weldx.constants.Q_ appears as a "pint.Quantity" when asdf inspects instances of weldx.constants.Q_ (as inspected with asdf.util.get_class_name).

I added some comments with a shorter version of the above description.

Related Issues

Fixes #879

Checks

  • updated CHANGELOG.md
  • updated tests/
  • updated doc/
  • update example/tutorial notebooks
  • update manifest file

in 0.22 Quantity appears at pint.registry.Quantity
and requires an update to the corresponding asdf
Converter (the same for pint.registry.Unit).
@github-actions
Copy link

github-actions bot commented Jun 2, 2023

Test Results

2 188 tests  ±0   2 187 ✔️ +202   2m 43s ⏱️ +27s
       1 suites ±0          1 💤 ±    0 
       1 files   ±0          0  - 197 

Results for commit 7e998ed. ± Comparison against base commit 65e6306.

♻️ This comment has been updated with latest results.

@codecov
Copy link

codecov bot commented Jun 2, 2023

Codecov Report

Merging #880 (7e998ed) into master (ac9ba0f) will decrease coverage by 0.02%.
The diff coverage is 92.30%.

@@            Coverage Diff             @@
##           master     #880      +/-   ##
==========================================
- Coverage   96.48%   96.47%   -0.02%     
==========================================
  Files          95       95              
  Lines        6293     6293              
==========================================
- Hits         6072     6071       -1     
- Misses        221      222       +1     
Impacted Files Coverage Δ
weldx/tags/units/pint_quantity.py 97.29% <ø> (ø)
weldx/util/xarray.py 96.15% <ø> (ø)
weldx/core/time_series.py 97.74% <66.66%> (ø)
weldx/constants.py 100.00% <100.00%> (ø)
weldx/core/generic_series.py 89.73% <100.00%> (ø)
weldx/time.py 97.88% <100.00%> (ø)
weldx/transformations/rotation.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

@CagtayFabry
Copy link
Member

thank you for the thorough explanation and solution with listing pint.Quantity as string and class for the converter @braingram

it is unfortunate that pint seems to be slightly unstable with the last releases in terms of typing and class refactoring.

I am waiting for the conda release of 0.22 here to merge this and update our dependencies and CI accordingly

@CagtayFabry CagtayFabry added units unit handling and pint dependencies changes in package dependencies labels Jun 3, 2023
@CagtayFabry CagtayFabry self-assigned this Jun 13, 2023
@CagtayFabry CagtayFabry merged commit 638078b into BAMWelDX:master Jun 13, 2023
@braingram braingram deleted the pint_quantity_path branch June 13, 2023 13:06
@braingram
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies changes in package dependencies units unit handling and pint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PintQuantityConverter error for newest pint
2 participants