duartegroup / autodE

automated reaction profile generation
https://duartegroup.github.io/autodE/
MIT License
166 stars 51 forks source link

Using autodE in Interactive python shell causes unusual change in units #239

Closed shoubhikraj closed 1 year ago

shoubhikraj commented 1 year ago

Only seen from interactive python shell.

>>> import autode as ade
>>> mol = ade.Molecule(smiles='CCO')
>>> mol.calc_hessian(method=ade.methods.XTB(),n_cores=6)
>>> mol.hessian.units
Unit(Ha Å^-2)
>>> mol.hessian.   # pressing tab twice to get all visible attributes
mol.hessian.T                  mol.hessian.min(
mol.hessian.all(               mol.hessian.n_tr
mol.hessian.any(               mol.hessian.n_v
mol.hessian.argmax(            mol.hessian.nbytes
mol.hessian.argmin(            mol.hessian.ndim
... # wall of text hidden
>>> mol.hessian.units  # units changed magically from Ha/ang^2 to Joule/ang^2?
Unit(J ang^-2)

I have looked at the values in the Hessian and the units really do change. (i.e. the numbers change as well) I am not quite sure why this is happening. But it would be problematic if the units changed randomly in the middle of calculations.

Environment

t-young31 commented 1 year ago

I can reproduce this – absolutely wild. no idea why this happens. thoughts appreciated!

shoubhikraj commented 1 year ago

@t-young31 Just found that the same behaviour can be reproduced by using inspect.

>>> inspect.getmembers(mol.hessian)
>>> mol.hessian.units
Unit(J ang^-2)

The code that getmembers is running is here: https://github.com/python/cpython/blob/498598e8c2d64232d26c075de87c513415176bbf/Lib/inspect.py#L555

So there is something there which is changing the units.

t-young31 commented 1 year ago

I reckon getmembers is evaluating the _mass_weighted cached_property, which converts to "Unit(J ang^-2)". It should probably take a copy right?

shoubhikraj commented 1 year ago

@t-young31 Yes, I think the issue is coming from the conversion procedure itself here: https://github.com/duartegroup/autodE/blob/6813afd7523091352634f37339e75c9cf95f232f/autode/values.py#L74-L77

I think that assigning to the original object's slice causes the original array object to be modified, instead of creating a new object.

t-young31 commented 1 year ago

I think that assigning to the original object's slice causes the original array object to be modified, instead of creating a new object.

I think that's the desired behaviour of to but a method that modifies an array in place should really put it back right!

shoubhikraj commented 1 year ago

@t-young31 I don't understand why to has to modify the underlying object, why not send back a modified copy:

new_value = value.__class__(np.array(value, copy=True) * c, units=units)
return new_value

Since to is returning something, there is no need to modify inplace.

t-young31 commented 1 year ago

It doesn't have to modify the object but in general unnecessary copies are bad.. pytorch/pandas offers both. I'm not sure either are 'better' (but definitely a method that modifies in-place to do something should revert it)