michellab / Sire

Sire Molecular Simulations Framework
http://siremol.org
GNU General Public License v3.0
95 stars 26 forks source link

Inconsistent behaviour of toVector for Python-wrapped array-like atom properties #406

Closed lohedges closed 2 years ago

lohedges commented 2 years ago

Ideally all array-like atom properties would behave in the same way, i.e. you could call .toVector() to return a Python list (or even numpy array) in an appropriate default unit. This works for the coordinates property, but doesn't for velocity.

For example, using BioSimSpace to load:

import BioSimSpace as BSS

system = BSS.IO.readMolecules(["pickled_system.gro", "pickled_system.top"])

coords = system._sire_object.property("coordinates")

print(coords, coords.isAnArray()
SireMol::AtomCoords() True

print(coords.toVector())
[( 2.87, -1.99, 0.94 ), ( 1.52, -2.32, 0.71 ), ( 0.51, -1.38, 0.98 ), ( 0.79, -0.17, 1.57 ), ( 2.09, 0.17, 1.8 ), ( 3.15, -0.75, 1.54 ), ( 4.55, -0.43, 1.88 ), ( 3.7, -2.65, 0.72 ), ( 1.36, -3.26, 0.19 ), ( -0.53, -1.59, 0.73 ), ( -0.03, 0.5, 1.8 ), ( 2.23, 1.14, 2.27 ), ( 5.04, 0.02, 1.02 ), ( 4.65, 0.24, 2.74 ), ( 5.15, -1.33, 2.09 ), ( 2.87, -1.99, 0.94 ), ( 1.52, -2.32, 0.71 ), ( 0.51, -1.38, 0.98 ), ( 0.79, -0.17, 1.57 ), ( 2.09, 0.17, 1.8 ), ( 3.15, -0.75, 1.54 ), ( 4.55, -0.43, 1.88 ), ( 3.7, -2.65, 0.72 ), ( 1.36, -3.26, 0.19 ), ( -0.53, -1.59, 0.73 ), ( -0.03, 0.5, 1.8 ), ( 2.23, 1.14, 2.27 )]

vels = system._sire_object.property("coordinates")
print(vels, vels.isAnArray()

AtomProperty<SireMol::Velocity3D>( [ ( 1.2e-05 angstrom fs-1, 0 angstrom fs-1, -1.8e-05 angstrom fs-1 ),( 6e-06 angstrom fs-1, -5e-06 angstrom fs-1, 2e-06 angstrom fs-1 ),( 4e-06 angstrom fs-1, -5e-06 angstrom fs-1, 4e-06 angstrom fs-1 ),( -5e-06 angstrom fs-1, -8e-06 angstrom fs-1, 6e-06 angstrom fs-1 ),( -3e-06 angstrom fs-1, 0 angstrom fs-1, -1.4e-05 angstrom fs-1 ),( -3e-06 angstrom fs-1, 1e-06 angstrom fs-1, -1.5e-05 angstrom fs-1 ),( 1.9e-05 angstrom fs-1, 0 angstrom fs-1, -9e-06 angstrom fs-1 ),( 2.3e-05 angstrom fs-1, 1.5e-05 angstrom fs-1, -2.5e-05 angstrom fs-1 ),( 2.3e-05 angstrom fs-1, 0 angstrom fs-1, -1.2e-05 angstrom fs-1 ),( 2e-06 angstrom fs-1, 7e-06 angstrom fs-1, 1e-06 angstrom fs-1 ),( -1.2e-05 angstrom fs-1, -2e-05 angstrom fs-1, 1.6e-05 angstrom fs-1 ),( -3.3e-05 angstrom fs-1, 6e-06 angstrom fs-1, -1.6e-05 angstrom fs-1 ),( -3e-06 angstrom fs-1, -8e-06 angstrom fs-1, -2.6e-05 angstrom fs-1 ),( 3e-06 angstrom fs-1, 0 angstrom fs-1, -6e-06 angstrom fs-1 ),( 2e-06 angstrom fs-1, -1.5e-05 angstrom fs-1, -2.1e-05 angstrom fs-1 ),( 1.2e-05 angstrom fs-1, 0 angstrom fs-1, -1.8e-05 angstrom fs-1 ),( 6e-06 angstrom fs-1, -5e-06 angstrom fs-1, 2e-06 angstrom fs-1 ),( 4e-06 angstrom fs-1, -5e-06 angstrom fs-1, 4e-06 angstrom fs-1 ),( -5e-06 angstrom fs-1, -8e-06 angstrom fs-1, 6e-06 angstrom fs-1 ),( -3e-06 angstrom fs-1, 0 angstrom fs-1, -1.4e-05 angstrom fs-1 ),( -3e-06 angstrom fs-1, 1e-06 angstrom fs-1, -1.5e-05 angstrom fs-1 ),( 1.9e-05 angstrom fs-1, 0 angstrom fs-1, -9e-06 angstrom fs-1 ),( 2.3e-05 angstrom fs-1, 1.5e-05 angstrom fs-1, -2.5e-05 angstrom fs-1 ),( 2.3e-05 angstrom fs-1, 0 angstrom fs-1, -1.2e-05 angstrom fs-1 ),( 2e-06 angstrom fs-1, 7e-06 angstrom fs-1, 1e-06 angstrom fs-1 ),( -1.2e-05 angstrom fs-1, -2e-05 angstrom fs-1, 1.6e-05 angstrom fs-1 ),( -3.3e-05 angstrom fs-1, 6e-06 angstrom fs-1, -1.6e-05 angstrom fs-1 ) ] ) True

print(vels.toVector())
TypeError: No to_python (by-value) converter found for C++ type: QVector<SireMaths::Vector3D<SireUnits::Dimension::PhysUnit<0, 1, -1, 0, 0, 0, 0> > >

Presumably the SireMol::AtomCoords type is handling the to toVector() conversion. There is a Siremol::AtomVelocities type, but casting the velocity above to this type makes no difference.

lohedges commented 2 years ago

Looking at the source code for AtomVelocities, it looks like the class hasn't been fleshed out. There is no cpp file with implementation details, as there is for AtomCoords.

chryswoods commented 2 years ago

AtomVelocities does have a toVector function. This is defined in atomproperty.hpp. Most AtomProperty-derived classes use the default implementation from atomproperty.hpp. AtomCoords is different, as it required lots of special cases, so ended up being a concrete manual specialisation of AtomProperty<Vector>.

The error message is saying that there isn't an automatic converter defined to convert a QVector<Velocity3D> from a C++ class to a Python class. This is because I'd forgotten to add the converter in SireMol_containers.cpp.

I have added more converters including the Velocity3D converter into the feat_2023_0 version of this file. You can merge this across, or just add the one-line addition to the devel version, e.g.

register_list< QVector<Velocity3D> >();

(remembering to #include "SireMol/atomvelocities.h" at the top of this file so that Velocity3D is defined)

lohedges commented 2 years ago

Brilliant, thanks for this. I'll add this to devel tomorrow and test.