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

Try and fix type annotations #68

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

Conversation

Yoshanuikabundi
Copy link
Contributor

Description

Not 100% sure but I think these # type: ignore messages are fixable. I'll see how many I can get through in this PR

Todos

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

  • TODO 1

Questions

  • Question1

Status

  • Ready to go

@Yoshanuikabundi
Copy link
Contributor Author

@mattwthompson What's the reason we do this:

@requires_package("openmm")
def _to_openmm(self) -> "OpenMMQuantity":
    """Convert the quantity to an ``openmm.unit.Quantity``.

    Returns
    -------
    openmm_quantity : openmm.unit.quantity.Quantity
        The OpenMM compatible quantity.
    """
    from openff.units.openmm import to_openmm

    return to_openmm(self)

Quantity.to_openmm = _to_openmm # type: ignore[attr-defined]

Instead of just writing to_openmm as a method of Quantity? I think mypy etc are going to struggle with this.

@mattwthompson
Copy link
Member

We choose to go to the Moon annotate our codebase in this decade and do the other things, not because they are easy, but because they are hard;

I did that not because I think it makes any sense to do but because it "works" and I'm mortified at learning why. Here's the paper trail: #67 (comment)

@Yoshanuikabundi
Copy link
Contributor Author

I have just spent an hour on this and I just want to say that I'm sorry I ever deigned to believe that I could sort this out.

I found Pint documentation on how to subclass registry, unit, and quantity, but PyRight and MyPy both don't seem to be able to figure out that the object produced by openff.unit.unit.Quantity(1) is a openff.unit.unit.Quantity, believing it to be a PlainQuantity from Pint instead. I think this might be a bug in PyRight but I'll look into it... later.

I'll push my changes to this branch and then it can sit here doing nothing for a bit while I finish sorting out the examples stuff.

@codecov-commenter
Copy link

codecov-commenter commented Jun 19, 2023

Codecov Report

Merging #68 (0b2e88d) into main (f2f27a7) will decrease coverage by 0.17%.
The diff coverage is 100.00%.

Additional details and impacted files

@mattwthompson
Copy link
Member

I've spent more time trying to wrestle this together and I have less to show for it. I'm not sure that's a bug in PyRight, I'm sure I'm deeply confused as to how Pint's internals work and I'm not sure it'd be simple to handle all of the different things that can be returned from UnitRegistry.Quantity. There doesn't seem to be a major drive to get things working upstream and/or something funky happens in how we subclass out. It's not a good position to be in.

I've considered trying to just make stubs to handle the cases our codebase uses, but that's not been as simple as I hoped.

@mattwthompson
Copy link
Member

I have mypy passing on 3.12 after #85

@mattwthompson
Copy link
Member

I think I have type annotations in a reasonable state on the main branch now - open to changes if I'm forgetting something

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