openforcefield / openff-toolkit

The Open Forcefield Toolkit provides implementations of the SMIRNOFF format, parameterization engine, and other tools. Documentation available at http://open-forcefield-toolkit.readthedocs.io
http://openforcefield.org
MIT License
309 stars 90 forks source link

VirtualSiteType.inPlaneAngle and outOfPlaneAngle._unit is None #1641

Open lilyminium opened 1 year ago

lilyminium commented 1 year ago

Describe the bug

Despite the code appearing to do otherwise, both VirtualSiteType.inPlaneAngle._unit and VirtualSiteType.outOfPlaneAngle._unit is None. Other unit-ed attributes, like VirtualSiteType.distance, don't have this issue. I'm not sure if this is actually a bug but I am a bit surprised. Relevant to #1637

Code here: https://github.com/openforcefield/openff-toolkit/blob/a53a7e1b29d6ec134114da517b17d19b34908602/openff/toolkit/typing/engines/smirnoff/parameters.py#L3368-L3370

To Reproduce

In [1]: import openff.toolkit

In [2]: openff.toolkit.__version__
Out[2]: '0.13.1+3.ga53a7e1b'

In [3]: ff = openff.toolkit.ForceField()

In [4]: handler = ff.get_parameter_handler("VirtualSites")

In [5]: handler.VirtualSiteType.inPlaneAngle._unit

In [6]: assert handler.VirtualSiteType.inPlaneAngle._unit is None

In [7]: assert handler.VirtualSiteType.outOfPlaneAngle._unit is not None
---------------------------------------------------------------------------
AssertionError                            Traceback (most recent call last)
Cell In[7], line 1
----> 1 assert handler.VirtualSiteType.outOfPlaneAngle._unit is not None

AssertionError:

In [8]: assert handler.VirtualSiteType.distance._unit is not None

Output

Computing environment (please complete the following information):

Additional context

mattwthompson commented 1 year ago

This is weird. There must be some special logic that is gunking this up, since there's nothing obvious about the base class or unit.


In [8]: ParameterAttribute(unit=unit.degree)._unit
Out[8]: <Unit('degree')>

In [9]: ParameterAttribute(unit=unit.degrees)._unit
Out[9]: <Unit('degree')>

In [10]: unit.degree.is_compatible_with(unit.degrees)
Out[10]: True

In [11]: unit.degree.dimensionality, unit.degrees.dimensionality
Out[11]: (<UnitsContainer({})>, <UnitsContainer({})>)
mattwthompson commented 1 year ago

I'm super stumped by this. I tried

These keywords aren't even hit often in other places in the code, so I'm worried there's something spooky happening deeper in the ParameterAttribute logic. These classes are hard for me to understand top-to-bottom, and I worry that changing anything will break everything, so I didn't explore that much.