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

Type checking of UnitRegistry instance calls are potentially broken, or return values are mistyped. #1642

Open
cgevans opened this issue Nov 1, 2022 · 3 comments
Labels

Comments

@cgevans
Copy link
Contributor

cgevans commented Nov 1, 2022

Changes in Quantity mean that basic methods on Quantity instances returned by calls to a UnitRegistry now result in typechecking errors in mypy. In our code, it breaks check and m_as. This does not occur if ureg.Quantity is called instead.

This may be caused by calls to UnitRegistry directly potentially not being Quantity instances (they can simply be numbers), and so the type checking here might be an improvement, but it also appears that those calls are documented as returning a Quantity ("Parse a mathematical expression including units and return a quantity object."), so there appears to be either a problem of function or of documentation here, and it appears that the type hinting within pint may be incorrectly stating that the calls return a Quantity.

This appears to be caused by c082656, and so is present in 0.19.3 and later.

A minimal example:

import pint
ureg = pint.UnitRegistry()
q = ureg("1 m")
q2 = ureg.Quantity("1 m")
q3 = ureg.Quantity("1")
q4 = ureg("1")
q.check("m")
q2.check("m")
q3.check("")
q4.check("")

Results in mypy error: error: Quantity? has no attribute "check" for q.check and q4.check, and no error for q2.check. q4.check also fails, because q4 is an int.

(Note that the Quantity to PlainQuantity change also breaks quite a bit of typechecking downstream, for anyone who was using a TypeVar with the quantity. However, this is fixable by replacing Quantity with PlainQuantity.)

@hgrecco
Copy link
Owner

hgrecco commented Nov 1, 2022

Let's first tackle the following questions:

  1. What should ureg.parse_expression return?
  2. What should ureg.__call__ do?

The answer to (1) has always been to parse the string as a math expression replacing names by their unit respective unit objects. This has not changed since Pint was started (except for the fact that in the beginning Unit and Quantity were just one object)

The answer to (2) has been call parse_expression since the registry became callable in 0.5

So indeed is type hinting of these functions what needs to be fixed. I guess they should return something like: Quantity | Unit | Number

Regarding the Quantity and PlainQuantity, as Quantity is properly defined could you point out what has been broken? My idea would be that all code using pint uses Quantity, and never PlainQuantity.

dcambie added a commit to cambiegroup/flowchem that referenced this issue Nov 1, 2022
@cgevans
Copy link
Contributor Author

cgevans commented Nov 1, 2022

The distinction is that PlainQuantity has a _MagnitudeType generic for the magnitude type. Quantity appears to have had this as well prior to 8209237, and so something like the following wouldn't result in a mypy error:

from typing import TypeVar
from pint import Quantity
T = TypeVar("T")
def ratio(top: Quantity[T], bottom: Quantity[T]) -> T:
    return (top / bottom).m_as("")

It now results in A function returning TypeVar should receive at least one argument containing the same Typevar, but does not with PlainQuantity, which does take a generic.

(This is linked to our using Decimal, of course. We try to ensure that floats don't sneak into our calculations in the wrong places.)

@hgrecco
Copy link
Owner

hgrecco commented Nov 2, 2022

Weird. Quantity has PlainQuantity as one of its parents. Maybe we have not subclassed in a way that is typing friendly?

(This is linked to our using Decimal, of course. We're try to ensure that floats don't sneak into our calculations in the wrong places.)

I fully support this. I am willing to make changes to bring this back!

@hgrecco hgrecco added the typing label Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants