espressomd / espresso

The ESPResSo package
https://espressomd.org
GNU General Public License v3.0
226 stars 183 forks source link

espresso assumes numpy for pint #4978

Closed PythonFZ closed 1 month ago

PythonFZ commented 1 month ago

When using

system.thermostat.set_langevin(kT=300 * ureg.K * boltmk, gamma=2, seed=42)

I receive

File /envs/espresso/lib/python3.10/site-packages/pint/facets/numpy/quantity.py:213, in NumpyQuantity.__len__(self)
    212 def __len__(self) -> int:
--> 213     return len(self._magnitude)

TypeError: object of type 'float' has no len()

but calling (300 * ureg.K * boltmk).magnitude directly works.

The same issue occurs at:

system.part.add(..., mass=atom.mass * ureg.u)
RudolfWeeber commented 1 month ago

I'm confused. Isn't teh value of kT evaluated immedaitely to a flot? Could you please provide the full traceback?

PythonFZ commented 1 month ago

This is the full traceback I can get out of espresso:

Traceback (most recent call last):
  File "/tmp/ipykernel_981524/1810778681.py", line 3, in <module>
    system.thermostat.set_langevin(kT=300 * ureg.K * boltmk, gamma=2, seed=42)
  File "script_interface.pyx", line 478, in espressomd.script_interface.ScriptInterfaceHelper.generate_caller.template_method
  File "script_interface.pyx", line 160, in espressomd.script_interface.PScriptInterface.call_method
  File "script_interface.pyx", line 280, in espressomd.script_interface.python_object_to_variant
  File "/data/fzills/miniconda3/envs/espresso/lib/python3.10/site-packages/pint/facets/numpy/quantity.py", line 213, in __len__
    return len(self._magnitude)
TypeError: object of type 'float' has no len()
jngrad commented 1 month ago

ESPResSo doesn't know what a Pint object is. We only allow a small set of types, and have to do a lot of guesswork for container types, especially when they contain mixed types. The source code is here, if you have the stomach for it.

In more details, Pint objects define an __iter__() function, so ESPResSo attempts to iterate them. A TypeError is raised in Pint version 0.18, but since 2021 they have progressively rolled out the new NumPy ndarray API over multiple releases to be more compatible with the NumPy ecosystem, with a big milestone with 0.20 when the facets/numpy/quantity.py was introduced.

In a nutshell, the numpy in your traceback was a red herring. An argument of type pint.Quantity was provided to a function whose docstring states only float is allowed (espressomd.thermostat.Thermostat.set_langevin()).

The easiest solution is to call the magnitude() method when passing arguments to ESPResSo functions.

The ESPResSo community has expressed an interest in using arguments with SI units in the past, however this would break backward compatibility, and the technical feasibility is unclear, especially considering we chose not to adopt the new NumPy ndarray API, and instead use the deprecated API (we cannot guarantee converting NumPy objects from ESPResSo to Pint quantities is safe in all situations, for example). The pyMBE project emerged to provide an alternative ESPResSo Python API which takes Pint objects as arguments and forwards them to ESPResSo by calling the magnitude() method, but it hasn't been fully ported to ESPResSo 4.3-dev yet. In my opinion, calling magnitude() everywhere necessary is your best option.

RudolfWeeber commented 1 month ago

OK, so pint.Quantity has iter even if it is not iterable.

One could argue, though, that pint is important enough to support it explicitly in python_object_to_variant() e.g.,

try: 
  import pint
  HAVE_PINT=True
except:
  HAVE_PINT=False

#...

if HAVE_PINT:
  if isinstance(value, pint.Quantity):
       return python_object_to_variant(value.magnitude)

or, if we don't want to import pint,

if type(value).__name__ == "Qauntity" and hasattr(value, "magnitude")
reinaual commented 1 month ago

Just a general comment: Pint stores the magnitude together with whatever 'unit' you put in. This does not have to be SI units or simulation units, just any unit system. However, Espresso expects simulation units, which it can never know if they are in the correct unit system. So, while it might sound intuitive to just give a pint.Quantity to Espresso, I think this also opens a very big pitfall since it's the user's responsibility to convert it to simulation units first before passing it to Espresso. Providing such an interface might suggest that Espresso can take care of the conversion, which it can't.

jngrad commented 1 month ago

Also:

I'm not opposed to adding Pint support to ESPResSo. But it's a big project. More urgent for us is planning the migration to NumPy 2.0 and Cython 3.10, and figuring out the limitations of our legacy Cython GIL release mechanism and deprecated NumPy ndarray API.

RudolfWeeber commented 1 month ago

My understanding is as follows: just supporting pint quantities without making ESPResSo unit-aware everywhere actually has unclear meaning.

Whether or not we should make ESPResSo as a whole unit-aware needs to be discussed separately.

Closing for now.