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

Add _quantity property for values with units #38

Merged
merged 3 commits into from
Sep 4, 2020

Conversation

jmuhlich
Copy link
Collaborator

@jmuhlich jmuhlich commented Aug 31, 2020

This still needs tests but I wanted to get your thoughts first. I didn't want to mess with the value fields and ..._unit fields so I added a parallel property with the _quantity suffix for all fields with units.

Ultimately it would be cleaner to replace the value fields with a pint.Quantity and hide the _unit fields completely (that approach would work especially well for create/modify operations), but I'm not sure how we'd want to do the type annotations in that case. Pint doesn't play so well with typing as it loads unit definitions at runtime -- all you can do is type stuff as Quantity. We can definitely get Pydantic to parse quantities and reject incompatible units, but mypy probably won't be any help.

closes #3

Copy link
Owner

@tlambert03 tlambert03 left a comment

Choose a reason for hiding this comment

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

oooh, lovely! I think this is a great intermediate step, before (/regardless of whether) we remove the unit field itself. I've never used pint before... this took much less code than I expected

what would you think of adding a setter to that quantity_property that accepts a pint.Quantity and extracts the magnitude and units? (not sure how hard that is for the few custom units you had to define here)

Repository owner deleted a comment from jmuhlich Aug 31, 2020
@jmuhlich
Copy link
Collaborator Author

jmuhlich commented Sep 1, 2020

A setter would work but again there wouldn't be any great way to type that. Mutable fields raises some other issues too -- I'll file another issue to discuss that.

@jmuhlich
Copy link
Collaborator Author

jmuhlich commented Sep 1, 2020

Oh, this is timely! The Pint main developer filed an issue to discuss annotations just the other day: hgrecco/pint#1166

@codecov-commenter
Copy link

codecov-commenter commented Sep 1, 2020

Codecov Report

Merging #38 into master will increase coverage by 0.00%.
The diff coverage is 97.72%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #38   +/-   ##
=======================================
  Coverage   95.20%   95.20%           
=======================================
  Files           1        1           
  Lines         396      480   +84     
=======================================
+ Hits          377      457   +80     
- Misses         19       23    +4     
Impacted Files Coverage Δ
src/ome_autogen.py 95.20% <97.72%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 059e0a4...46cda8d. Read the comment docs.

@jmuhlich
Copy link
Collaborator Author

jmuhlich commented Sep 2, 2020

I added one test to get a feel for what we might want to check. I didn't use the model-as-fixture approach since we saw that introduced problems in #34.

I originally tried specifying those two length values in different units, but I ran into float precision issues due to the way Pint manages conversions internally (the difference would end up being like 54.99999999999993 nm instead of exactly 55). Pint does support building the UnitRegistry with Decimal factors instead of float which would yield exact values, but then all Quantities would need to be initialized with int or Decimal values and users would need to convert magnitudes to float before doing any math with other floats (Decimals were deliberately designed not to interoperate with floats to avoid silent loss of precision). I can cook something up to make this work in the test (like comparing with a tolerance or rounding first) but I wonder if users are going to be confused if they can't convert e.g. 123 nm to pm and get 123000 exactly.

>>> ureg.Quantity(123, 'nm').to('pm').m                                                                      
123000.00000000001

Maybe this isn't a big issue in practice and I'm overthinking it. Thoughts?

@tlambert03
Copy link
Owner

I can cook something up to make this work in the test (like comparing with a tolerance or rounding first) but I wonder if users are going to be confused if they can't convert e.g. 123 nm to pm and get 123000 exactly.

yeah, it's slightly annoying... but I think it might be more annoying to have to use Decimal objects everywhere. so it's fine with me to leave this as a "maybe someday if we or someone else complains" and move on for now. rounding for tests purposes works for me

@jmuhlich
Copy link
Collaborator Author

jmuhlich commented Sep 3, 2020

Does this look like enough tests? I wanted to avoid testing too much and end up just testing Pint itself. Also do you feel dataclasses is the right place for the ureg?

@jmuhlich jmuhlich marked this pull request as ready for review September 3, 2020 18:09
Copy link
Owner

@tlambert03 tlambert03 left a comment

Choose a reason for hiding this comment

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

yep, I'm very happy with this! tests are great for now. and I'm also fine leaving ureg in dataclasses (unless you think it fits better in util). Thanks again for this... it's super nice. I'll let you merge when ready

@jmuhlich jmuhlich merged commit 90c1e3c into tlambert03:master Sep 4, 2020
@jmuhlich jmuhlich deleted the units branch September 4, 2020 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert units to proper python objects with (e.g.) Pint
3 participants