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

QoL improvements for IDEs and LSPs #41

Merged
merged 3 commits into from
Sep 12, 2022
Merged

Conversation

Yoshanuikabundi
Copy link
Contributor

Description

I ran into a few minor barbs working with units in the Toolkit using the PyRight LSP. PyRight includes a type checker that is generally stricter than MyPy and less pedantic than MyPy --strict, so I love it. This PR fixes those barbs.

Todos

Notable points that this PR has either accomplished or will accomplish.

  • Add __all__ to the exceptions module.
    • This helps IDEs understand that the exceptions are part of the public API.
  • Make Quantity generic over its magnitude type.
    • This allows users to specify the magnitude type as part of a type annotation, which is very useful. Previously, an OpenFF Quantity was always a Quantity[Any].
    • MyPy seems to be fine with specifying a generic for a non-generic type, but Python itself is not (it causes a TypeError).

Status

  • Ready to go

Inheriting from a generic type without specifying the generic
fixes the generic to `Any`. This breaks strict type checking
and prevents users from specifying the type that a Quantity is
wrapping.
@codecov-commenter
Copy link

codecov-commenter commented Aug 17, 2022

Codecov Report

Merging #41 (6608ec3) into main (b04bd4d) will increase coverage by 0.09%.
The diff coverage is 100.00%.

Copy link
Member

@mattwthompson mattwthompson left a comment

Choose a reason for hiding this comment

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

I don't like defining __all__ (I'd ban star imports if I could and it rapidly goes out of sync) but not enough stand in the way of making an IDE happy.

Could you explain _MagnitudeType, perhaps in other language or a with an example? I'm hung up on what "magnitude type" means - is this int vs. float vs numpy.ndarray, etc.? I see bracketed Quantitys and think of the length: FloatQuantity["nanometer"] = Field(..) annotations we have in most of our Pydantic models. IME the data wrapped with units are typically ducky so I have not given this much thought or seen the utility.

I guess the intent here is that you can distinguish Quantity[int] from Quantity[numpy.ndarray]? If so, I see the value in at least distinguishing scalar and matrix values ... would a type checker actually pick up on disobeying the original annotation?

@Yoshanuikabundi
Copy link
Contributor Author

Yoshanuikabundi commented Aug 22, 2022

Agree with you on star imports. I think the way to think about how __all__ functions in the tooling/IDE world is that it specifies the public API. Without it, the only way to distinguish a re-export from an import is to rename the import with a leading underscore (from x import y as _y). I think it's good to specify this, as it makes it clear what to document publicly, what we promise not to break, and it avoids cluttering people's autocompletes with our imports.

is this int vs. float vs numpy.ndarray, etc.?

Yeah exactly, the magnitude type is the data type wrapped by the quantity, the type you get when you call Quantity.magnitude() or Quantity.m():

box_vectors: Quantity[ArrayLike]
formal_charge: Quantity[float]

I see bracketed Quantitys and think of the length: FloatQuantity["nanometer"] = Field(..) annotations we have in most of our Pydantic models.

I would love this feature (though I'd want to specify dimensions and not units. Or both/either I guess), and there is an open issue for it on Pint, but it hasn't seen much attention recently. One day I want to specify types like box_vectors: Quantity["length", NDArray[(3, 3), float]]

would a type checker actually pick up on disobeying the original annotation?

It does for Pint quantities, but doesn't seem happy with OpenFF Quantities. This appears to be a problem with the status quo, at least in PyRight. So I guess I jumped the gun on this PR and I need to figure out what's happening there! I'll take another look after I've finished the toolkit showcase PR.

Pint quantities work as expected in both pyright and mypy:

from openff.units import unit
from pint import Quantity as PintQuantity
from openff.units import Quantity as OpenFFQuantity
from numpy.typing import NDArray
import numpy as np

# Create pint quantities of ints and arrays
pint_quant_int = PintQuantity(1, unit.nanometer)
pint_quant_ary = PintQuantity(np.array([1]), unit.nanometer)
# Assign the above to annotated variables
pint_quant_int_annotated_ary: PintQuantity[NDArray] = pint_quant_int # Should not typecheck
pint_quant_int_annotated_int: PintQuantity[int] = pint_quant_int # Should typecheck
pint_quant_ary_annotated_ary: PintQuantity[NDArray] = pint_quant_ary # Should not typecheck
pint_quant_ary_annotated_int: PintQuantity[int] = pint_quant_ary # should typecheck

MyPy:

openff/units/type_test.py:11: error: Incompatible types in assignment (expression has type "Quantity[int]", variable has type "Quantity[ndarray[Any, dtype[Any]]]")
openff/units/type_test.py:14: error: Incompatible types in assignment (expression has type "Quantity[ndarray[Any, dtype[Any]]]", variable has type "Quantity[int]")
Found 2 errors in 1 file (checked 14 source files)

PyRight:

/home/joshmitchell/Documents/openff/units/openff/units/type_test.py
  /home/joshmitchell/Documents/openff/units/openff/units/type_test.py:11:55 - error: Expression of type "Quantity[int]" cannot be assigned to declared type "Quantity[NDArray[Unknown]]"
    TypeVar "_MagnitudeType@Quantity" is invariant
      "int" is incompatible with "NDArray[Unknown]" (reportGeneralTypeIssues)
  /home/joshmitchell/Documents/openff/units/openff/units/type_test.py:14:51 - error: Expression of type "Quantity[NDArray[Any]]" cannot be assigned to declared type "Quantity[int]"
    TypeVar "_MagnitudeType@Quantity" is invariant
      "NDArray[Any]" is incompatible with "int" (reportGeneralTypeIssues)

But I'm glad you brought this up explicitly because it seems MyPy and PyRight both seem not to like the way type annotations are inherited here:

from openff.units import unit
from pint import Quantity as PintQuantity
from openff.units import Quantity as OpenFFQuantity
from numpy.typing import NDArray
import numpy as np

# Same for OpenFF
openff_quant_int = OpenFFQuantity(1, unit.nanometer)
openff_quant_ary = OpenFFQuantity(np.array([1]), unit.nanometer)
openff_quant_int_annotated_ary: OpenFFQuantity[NDArray] = openff_quant_int
openff_quant_int_annotated_int: OpenFFQuantity[int] = openff_quant_int
openff_quant_ary_annotated_ary: OpenFFQuantity[NDArray] = openff_quant_ary
openff_quant_ary_annotated_int: OpenFFQuantity[int] = openff_quant_ary

# Try leaving off the generic
openff_quant_int_annotated_quant: OpenFFQuantity = openff_quant_int

MyPy can't seem to find the type annotation on the parent type:

openff/units/type_test.py:8: error: Need type annotation for "openff_quant_int"
openff/units/type_test.py:9: error: Need type annotation for "openff_quant_ary"
Found 2 errors in 1 file (checked 14 source files)

PyRight finds the type annotation, but seems not to accept OpenFFQuantity as an instance of PintQuantity, leading to confusing messages (it's also unhappy with the status quo):

/home/joshmitchell/Documents/openff/units/openff/units/type_test.py
  /home/joshmitchell/Documents/openff/units/openff/units/type_test.py:10:59 - error: Expression of type "Quantity[int]" cannot be assigned to declared type "Quantity[NDArray[Unknown]]"
    "Quantity[int]" is incompatible with "Quantity[NDArray[Unknown]]" (reportGeneralTypeIssues)
  /home/joshmitchell/Documents/openff/units/openff/units/type_test.py:11:55 - error: Expression of type "Quantity[int]" cannot be assigned to declared type "Quantity[int]"
    "pint.quantity.Quantity" is incompatible with "openff.units.units.Quantity" (reportGeneralTypeIssues)
  /home/joshmitchell/Documents/openff/units/openff/units/type_test.py:12:59 - error: Expression of type "Quantity[NDArray[Any]]" cannot be assigned to declared type "Quantity[NDArray[Unknown]]"
    "Quantity[NDArray[Any]]" is incompatible with "Quantity[NDArray[Unknown]]" (reportGeneralTypeIssues)
  /home/joshmitchell/Documents/openff/units/openff/units/type_test.py:13:55 - error: Expression of type "Quantity[NDArray[Any]]" cannot be assigned to declared type "Quantity[int]"
    "Quantity[NDArray[Any]]" is incompatible with "Quantity[int]" (reportGeneralTypeIssues)
  /home/joshmitchell/Documents/openff/units/openff/units/type_test.py:16:52 - error: Expression of type "Quantity[int]" cannot be assigned to declared type "Quantity[Unknown]"
    "Quantity[int]" is incompatible with "Quantity[Unknown]" (reportGeneralTypeIssues)
5 errors, 0 warnings, 0 informations 

@mattwthompson mattwthompson merged commit ba3d5b9 into main Sep 12, 2022
@mattwthompson mattwthompson deleted the ide_usability_fixes branch September 13, 2022 01:23
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.

3 participants