-
Notifications
You must be signed in to change notification settings - Fork 100
Improve some typing #2000
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
base: main
Are you sure you want to change the base?
Improve some typing #2000
Conversation
Codecov ReportAttention: Patch coverage is
🚀 New features to boost your workflow:
|
Okey - sorry to screw up your PR briefly, I think I've brought it up to date with the non-trivial changes that have been happening upstream. I have some ideas about how we can revamp typing in general, as I'm sure you do, but this is neither the time or the place for me to push that |
7d7f1c8
to
3dc07df
Compare
2b59dd4
to
e906ff2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Process-wise, this is an "approve" in terms of what looks good. But I found that I broke CI at some point and Mypy isn't actually running. I would like to see it running in these CI checks (ideally on the most recent version, 1.17.0 I think) before merging this. #2087 or something similar should do it.
I have a few other comments which you are welcome to take or leave. This was fun to review and I like seeing these improvements trickle into our codebase!
The stereochemistry around this atom, allowed values are "CW", "CCW", or None, | ||
The stereochemistry around this atom, allowed values are "R", "S", or None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related #2084
@@ -14,6 +14,8 @@ Releases follow the `major.minor.micro` scheme recommended by [PEP440](https://w | |||
|
|||
### Bugfixes | |||
- [PR #2052](https://github.com/openforcefield/openff-toolkit/pull/2052): Fixes bug where `Topology.from_pdb` couldn't load NH4+ ([Issue #2051](https://github.com/openforcefield/openff-toolkit/issues/2051)) | |||
- [PR #2000](https://github.com/openforcefield/openff-toolkit/pull/2000): Assorted type annotation fixes | |||
- [PR #2000](https://github.com/openforcefield/openff-toolkit/pull/2000): `Molecule.to_rdkit()` no longer raises an exception when converting a molecule with non-decimal residue numbers to RDKit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could use a line here about what happens in this case since the behavior is new and not obviously well-defined
off_atom.metadata["residue_name"] = atom.residue.name # type:ignore[attr-defined] | ||
off_atom.metadata["residue_number"] = atom.residue.id # type:ignore[attr-defined] | ||
off_atom.metadata["insertion_code"] = atom.residue.insertionCode # type:ignore[attr-defined] | ||
off_atom.metadata["chain_id"] = atom.residue.chain.id # type:ignore[attr-defined] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought a little bit about how to make this cleaner, could only come up with using getattr
which isn't actually any cleaner. Remind me that all of these attributes being optional is (intentionally) part of the design?
@@ -2343,7 +2343,8 @@ def atom(self, atom_topology_index: int) -> "Atom": | |||
if next_molecule_start_index > atom_topology_index: | |||
atom_molecule_index = atom_topology_index - this_molecule_start_index | |||
# NOTE: the index here should still be in the topology index order, NOT the reference molecule's | |||
return molecule.atom(atom_molecule_index) | |||
# can Molecule.atom be a _SimpleAtom? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No I ... uhh ... hope not.
@@ -131,7 +131,7 @@ def assign_partial_charges( | |||
partial_charges = [0.0] * molecule.n_atoms | |||
|
|||
elif partial_charge_method == "formal_charge": | |||
partial_charges = [float(atom.formal_charge.m) for atom in molecule.atoms] | |||
partial_charges = [float(atom.formal_charge.m) for atom in molecule.atoms] # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the error code here?
I won't be picky enough to ask here, but in a perfect world I'd love to see us always have the specific error code in these ignores. Mypy can enforce this
aromatic_matching=kwargs.get("aromatic_matching", True), | ||
formal_charge_matching=kwargs.get("formal_charge_matching", True), | ||
bond_order_matching=kwargs.get("bond_order_matching", True), | ||
atom_stereochemistry_matching=kwargs.get( | ||
"atom_stereochemistry_matching", True | ||
), | ||
bond_stereochemistry_matching=kwargs.get( | ||
"bond_stereochemistry_matching", True | ||
), | ||
strip_pyrimidal_n_atom_stereo=kwargs.get( | ||
"strip_pyrimidal_n_atom_stereo", True | ||
), | ||
toolkit_registry=kwargs.get("toolkit_registry", GLOBAL_TOOLKIT_REGISTRY), | ||
aromatic_matching=aromatic_matching, | ||
formal_charge_matching=formal_charge_matching, | ||
bond_order_matching=bond_order_matching, | ||
atom_stereochemistry_matching=atom_stereochemistry_matching, | ||
bond_stereochemistry_matching=bond_stereochemistry_matching, | ||
strip_pyrimidal_n_atom_stereo=strip_pyrimidal_n_atom_stereo, | ||
toolkit_registry=toolkit_registry, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it
@@ -2879,7 +2885,7 @@ def to_networkx(self) -> "nx.Graph": | |||
""" | |||
import networkx as nx | |||
|
|||
G: nx.classes.graph.Graph = nx.Graph() | |||
G: nx.classes.graph.Graph[int] = nx.Graph() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woah! Does this mean they've implemented it as a generic?
I'm working on upstream fixes in #2087, will update here when that's done or if I decide to pull the plug |
#2087 is a mess. In the spirit of keeping the ball rolling, I'm okay merging this as-is |
"""Set the atoms stereochemistry | ||
Parameters | ||
---------- | ||
value | ||
The stereochemistry around this atom, allowed values are "CW", "CCW", or None, | ||
The stereochemistry around this atom, allowed values are "R", "S", or None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The stereochemistry around this atom, allowed values are "R", "S", or None, | |
The stereochemistry around this atom, suggested values are "R", "S", or None, |
This PR includes many changes needed for OpenFF Pablo to type check, and 1 for it to run correctly. Pablo development so far has and will continue to test and type check against this branch (or
main
once it is merged) until it gets into a release.Molecule.to_rdkit()
is run on a molecule with residue numbers that cannot be converted toint
(such as hexadecimal residue numbers used by OpenMM in large PDB files)Molecule.is_isomorphic_with()
so that its keyword arguments are actually keyword arguments. This is not a breaking change as the**kwargs
argument is still present, it's just ignored - we should deprecate and remove this ASAPSorry for the behemoth PR!