From 5412f1c5080f4aec7943be4401f9e07368367d25 Mon Sep 17 00:00:00 2001 From: Christopher Woods Date: Wed, 13 Dec 2023 17:55:32 +0000 Subject: [PATCH] Backport of the fix to correctly set the cutoff value if a string is passed Also adds PropertyMap::unset to unset map properties --- corelib/src/libs/SireBase/propertymap.cpp | 7 ++ corelib/src/libs/SireBase/propertymap.h | 2 + .../src/libs/SireSystem/forcefieldinfo.cpp | 9 ++ doc/source/changelog.rst | 6 ++ src/sire/mol/_dynamics.py | 16 +++ src/sire/system/__init__.py | 6 ++ tests/mol/test_dynamics.py | 27 +++++ wrapper/Base/CMakeAutogenFile.txt | 102 +++++++++--------- wrapper/Base/Properties.pypp.cpp | 12 ++- wrapper/Base/PropertyMap.pypp.cpp | 13 +++ wrapper/Base/SireBase_properties.cpp | 20 ++-- wrapper/Base/SireBase_registrars.cpp | 56 +++++----- wrapper/Convert/__init__.py | 17 +++ 13 files changed, 199 insertions(+), 94 deletions(-) diff --git a/corelib/src/libs/SireBase/propertymap.cpp b/corelib/src/libs/SireBase/propertymap.cpp index 3f7684467..7b5511bc2 100644 --- a/corelib/src/libs/SireBase/propertymap.cpp +++ b/corelib/src/libs/SireBase/propertymap.cpp @@ -405,6 +405,13 @@ bool PropertyMap::specified(const PropertyName &propname) const return propname.hasValue(); } +/** Unset the property called 'name'. This will return it to default */ +void PropertyMap::unset(const QString &name) +{ + if (propmap.contains(name)) + propmap.remove(name); +} + /** Set the property called 'name' to have the source or value in 'source'. This replaces any existing source or value for any existing property of this name in this map */ diff --git a/corelib/src/libs/SireBase/propertymap.h b/corelib/src/libs/SireBase/propertymap.h index e9eecabbf..b6303ba45 100644 --- a/corelib/src/libs/SireBase/propertymap.h +++ b/corelib/src/libs/SireBase/propertymap.h @@ -206,6 +206,8 @@ namespace SireBase void set(const QString &name, const PropertyName &source); + void unset(const QString &name); + PropertyMap addPrefix(const QString &prefix, const QStringList &properties) const; diff --git a/corelib/src/libs/SireSystem/forcefieldinfo.cpp b/corelib/src/libs/SireSystem/forcefieldinfo.cpp index 5f665031b..de452c0d3 100644 --- a/corelib/src/libs/SireSystem/forcefieldinfo.cpp +++ b/corelib/src/libs/SireSystem/forcefieldinfo.cpp @@ -190,6 +190,15 @@ ForceFieldInfo::ForceFieldInfo(const System &system, { this->setCutoff(cutoff_prop.value().asA().toUnit()); } + else if (cutoff_prop.source() != "cutoff") + { + throw SireError::invalid_arg(QObject::tr( + "The cutoff property should have a value. It cannot be the string " + "'%1'. If you want to specify the cutoff type, using " + "the 'cutoff_type' property.") + .arg(cutoff_prop.source()), + CODELOC); + } const auto cutoff_type = map["cutoff_type"]; diff --git a/doc/source/changelog.rst b/doc/source/changelog.rst index 0a8e62388..ee0216c04 100644 --- a/doc/source/changelog.rst +++ b/doc/source/changelog.rst @@ -43,6 +43,12 @@ organisation on `GitHub `__. * Fixed the bug where the wrong return type from ``.minimisation()`` and ``.dynamics()`` was returned. This fixes issue #137. +* Fixed the bug where the cutoff would not be set correctly if a string + was passed. You can now do ``mol.dynamics(cutoff="10A")`` or + ``mol.dynamics(cutoff="infinite")`` and it will be processed correctly. + This also required adding a ``map.unset("key")`` option to ``PropertyMap``, + to make it easier to unset mapped properties. + `2023.4.1 `__ - October 2023 --------------------------------------------------------------------------------------------- diff --git a/src/sire/mol/_dynamics.py b/src/sire/mol/_dynamics.py index 03b7fb4c0..f4816197b 100644 --- a/src/sire/mol/_dynamics.py +++ b/src/sire/mol/_dynamics.py @@ -35,6 +35,22 @@ def __init__(self, mols=None, map=None, **kwargs): mols.atoms().find(selection_to_atoms(mols, fixed_atoms)), ) + if map.specified("cutoff"): + cutoff = map["cutoff"] + + if cutoff.has_source(): + cutoff = cutoff.source() + + if cutoff.lower() == "none" or cutoff.lower().startswith("infinit"): + self._map.set("cutoff_type", "NONE") + self._map.unset("cutoff") + elif cutoff.lower() == "auto": + self._map.unset("cutoff") + elif cutoff != "cutoff": + from .. import u + + self._map.set("cutoff", u(cutoff)) + # get the forcefield info from the passed parameters # and from whatever we can glean from the molecules from ..system import ForceFieldInfo, System diff --git a/src/sire/system/__init__.py b/src/sire/system/__init__.py index bf27ecf91..58ede02d6 100644 --- a/src/sire/system/__init__.py +++ b/src/sire/system/__init__.py @@ -22,6 +22,12 @@ def __init__(self, val, map=None): super().__init__(val, map=map) + def __repr__(self): + return self.__str__() + + def __str__(self): + return super().__str__() + _use_new_api() diff --git a/tests/mol/test_dynamics.py b/tests/mol/test_dynamics.py index 0d8af3286..78842ee75 100644 --- a/tests/mol/test_dynamics.py +++ b/tests/mol/test_dynamics.py @@ -48,3 +48,30 @@ def test_minimisation_return_type(ala_mols): a = atom.minimisation(platform="Reference").run(1).commit() assert isinstance(a, type(atom)) + + +@pytest.mark.skipif( + "openmm" not in sr.convert.supported_formats(), + reason="openmm support is not available", +) +def test_cutoff_options(ala_mols): + mols = ala_mols + + d = mols.dynamics(platform="Reference", cutoff="10 A") + + assert d.info().cutoff() == sr.u("10 A") + + d = mols[0].dynamics(platform="Reference", cutoff="infinite", vacuum=True) + + # OpenMM cannot have no cutoff, so sets it to a very large number + assert d.info().cutoff() > sr.u("1000 A") + + d = mols[0].dynamics(platform="Reference", cutoff="none", vacuum=True) + + # OpenMM cannot have no cutoff, so sets it to a very large number + assert d.info().cutoff() > sr.u("1000 A") + + d = mols.dynamics(platform="Reference", cutoff=sr.u("7.5A"), cutoff_type="PME") + + assert d.info().cutoff() == sr.u("7.5 A") + assert d.info().cutoff_type() == "PME" diff --git a/wrapper/Base/CMakeAutogenFile.txt b/wrapper/Base/CMakeAutogenFile.txt index 32de73938..5995b1747 100644 --- a/wrapper/Base/CMakeAutogenFile.txt +++ b/wrapper/Base/CMakeAutogenFile.txt @@ -1,68 +1,68 @@ # WARNING - AUTOGENERATED FILE - CONTENTS WILL BE OVERWRITTEN! set ( PYPP_SOURCES - Array2DBase.pypp.cpp - Array2D_double_.pypp.cpp - ArrayProperty_QString_.pypp.cpp - ArrayProperty_double_.pypp.cpp - ArrayProperty_int_.pypp.cpp - BooleanProperty.pypp.cpp - CPUID.pypp.cpp - ChunkedVector_double_.pypp.cpp - CombineProperties.pypp.cpp + TrigArray2DBase.pypp.cpp DoubleArrayProperty.pypp.cpp - FlopsMark.pypp.cpp - GeneralUnitArrayProperty.pypp.cpp - GeneralUnitProperty.pypp.cpp - Incremint.pypp.cpp - IntegerArrayProperty.pypp.cpp - LazyEvaluator.pypp.cpp - LengthProperty.pypp.cpp - LinkToProperty.pypp.cpp - LowerCaseString.pypp.cpp - MajorMinorVersion.pypp.cpp - MemInfo.pypp.cpp - NoMangling.pypp.cpp NullProperty.pypp.cpp - NumberProperty.pypp.cpp - PackedArray2D_DoubleArrayProperty.pypp.cpp - PackedArray2D_DoubleArrayProperty_Array.pypp.cpp - PackedArray2D_IntegerArrayProperty.pypp.cpp - PackedArray2D_IntegerArrayProperty_Array.pypp.cpp - PackedArray2D_PropertyList.pypp.cpp PackedArray2D_PropertyList_Array.pypp.cpp - PackedArray2D_QString_.pypp.cpp + Process.pypp.cpp + CombineProperties.pypp.cpp + _Base_free_functions.pypp.cpp PackedArray2D_QString_Array.pypp.cpp - PackedArray2D_QVariant_.pypp.cpp - PackedArray2D_QVariant_Array.pypp.cpp - PackedArray2D_StringArrayProperty.pypp.cpp - PackedArray2D_StringArrayProperty_Array.pypp.cpp - PackedArray2D_double_.pypp.cpp - PackedArray2D_double_Array.pypp.cpp - PackedArray2D_int_.pypp.cpp + PackedArray2D_IntegerArrayProperty.pypp.cpp + ArrayProperty_int_.pypp.cpp + PackedArray2D_IntegerArrayProperty_Array.pypp.cpp + ArrayProperty_double_.pypp.cpp + Incremint.pypp.cpp + PackedArray2D_DoubleArrayProperty_Array.pypp.cpp + PackedArray2D_DoubleArrayProperty.pypp.cpp + GeneralUnitProperty.pypp.cpp + vector_less__double__greater_.pypp.cpp PackedArray2D_int_Array.pypp.cpp - Process.pypp.cpp - ProgressBar.pypp.cpp - Properties.pypp.cpp - Property.pypp.cpp - PropertyList.pypp.cpp - PropertyMap.pypp.cpp - PropertyName.pypp.cpp + TrimString.pypp.cpp + FlopsMark.pypp.cpp + NoMangling.pypp.cpp + GeneralUnitArrayProperty.pypp.cpp Range.pypp.cpp - SimpleRange.pypp.cpp - StringArrayProperty.pypp.cpp - StringMangler.pypp.cpp + LengthProperty.pypp.cpp + ChunkedVector_double_.pypp.cpp StringProperty.pypp.cpp + PackedArray2D_QString_.pypp.cpp TempDir.pypp.cpp + PackedArray2D_double_.pypp.cpp + PropertyName.pypp.cpp TimeProperty.pypp.cpp - TrigArray2DBase.pypp.cpp - TrigArray2D_double_.pypp.cpp - TrimString.pypp.cpp + Array2DBase.pypp.cpp + NumberProperty.pypp.cpp + LinkToProperty.pypp.cpp UnitTest.pypp.cpp - UpperCaseString.pypp.cpp + PackedArray2D_PropertyList.pypp.cpp + ProgressBar.pypp.cpp + PropertyMap.pypp.cpp + LowerCaseString.pypp.cpp + PackedArray2D_double_Array.pypp.cpp + Properties.pypp.cpp + PropertyList.pypp.cpp + TrigArray2D_double_.pypp.cpp + PackedArray2D_StringArrayProperty.pypp.cpp VariantProperty.pypp.cpp + ArrayProperty_QString_.pypp.cpp + CPUID.pypp.cpp + UpperCaseString.pypp.cpp + PackedArray2D_QVariant_Array.pypp.cpp + BooleanProperty.pypp.cpp + PackedArray2D_StringArrayProperty_Array.pypp.cpp Version.pypp.cpp - _Base_free_functions.pypp.cpp - vector_less__double__greater_.pypp.cpp + SimpleRange.pypp.cpp + Array2D_double_.pypp.cpp + PackedArray2D_QVariant_.pypp.cpp + MemInfo.pypp.cpp + IntegerArrayProperty.pypp.cpp + StringArrayProperty.pypp.cpp + LazyEvaluator.pypp.cpp + Property.pypp.cpp + MajorMinorVersion.pypp.cpp + StringMangler.pypp.cpp + PackedArray2D_int_.pypp.cpp SireBase_containers.cpp SireBase_properties.cpp SireBase_registrars.cpp diff --git a/wrapper/Base/Properties.pypp.cpp b/wrapper/Base/Properties.pypp.cpp index f8626eeb6..ae40b6f4d 100644 --- a/wrapper/Base/Properties.pypp.cpp +++ b/wrapper/Base/Properties.pypp.cpp @@ -24,6 +24,8 @@ namespace bp = boost::python; #include +#include + #include "properties.h" SireBase::Properties __copy__(const SireBase::Properties &other){ return SireBase::Properties(other); } @@ -53,7 +55,7 @@ void register_Properties_class(){ , addLink_function_value , ( bp::arg("key"), bp::arg("linked_property") ) , bp::release_gil_policy() - , "" ); + , "Add a link from the property key to the property linked_property.\n The linked_property will be returned if there is no property\n called key in this set.\n\n Note that the linked property must already be contained in this set.\n" ); } { //::SireBase::Properties::allMetadata @@ -153,7 +155,7 @@ void register_Properties_class(){ "getLinks" , getLinks_function_value , bp::release_gil_policy() - , "" ); + , "Return all of the property links" ); } { //::SireBase::Properties::hasLinks @@ -165,7 +167,7 @@ void register_Properties_class(){ "hasLinks" , hasLinks_function_value , bp::release_gil_policy() - , "" ); + , "Return whether or not there are any property links" ); } { //::SireBase::Properties::hasMetadata @@ -422,7 +424,7 @@ void register_Properties_class(){ "removeAllLinks" , removeAllLinks_function_value , bp::release_gil_policy() - , "" ); + , "Remove all property links from this set" ); } { //::SireBase::Properties::removeAllMetadata @@ -460,7 +462,7 @@ void register_Properties_class(){ , removeLink_function_value , ( bp::arg("key") ) , bp::release_gil_policy() - , "" ); + , "Remove the link associated with the key key" ); } { //::SireBase::Properties::removeMetadata diff --git a/wrapper/Base/PropertyMap.pypp.cpp b/wrapper/Base/PropertyMap.pypp.cpp index c6332f52d..b16f7f7ab 100644 --- a/wrapper/Base/PropertyMap.pypp.cpp +++ b/wrapper/Base/PropertyMap.pypp.cpp @@ -226,6 +226,19 @@ void register_PropertyMap_class(){ , bp::release_gil_policy() , "" ); + } + { //::SireBase::PropertyMap::unset + + typedef void ( ::SireBase::PropertyMap::*unset_function_type)( ::QString const & ) ; + unset_function_type unset_function_value( &::SireBase::PropertyMap::unset ); + + PropertyMap_exposer.def( + "unset" + , unset_function_value + , ( bp::arg("name") ) + , bp::release_gil_policy() + , "Unset the property called name. This will return it to default" ); + } { //::SireBase::PropertyMap::what diff --git a/wrapper/Base/SireBase_properties.cpp b/wrapper/Base/SireBase_properties.cpp index df6b794cc..e2880ae6d 100644 --- a/wrapper/Base/SireBase_properties.cpp +++ b/wrapper/Base/SireBase_properties.cpp @@ -4,6 +4,15 @@ #include "Base/convertproperty.hpp" #include "SireBase_properties.h" +#include "SireStream/datastream.h" +#include "range.h" +#include "ranges.h" +#include "range.h" +#include "SireStream/datastream.h" +#include "SireStream/shareddatastream.h" +#include "stringmangler.h" +#include +#include "stringmangler.h" #include "SireError/errors.h" #include "SireError/getbacktrace.h" #include "SireStream/datastream.h" @@ -14,18 +23,9 @@ #include #include #include "property.h" -#include "SireStream/datastream.h" -#include "range.h" -#include "ranges.h" -#include "range.h" -#include "SireStream/datastream.h" -#include "SireStream/shareddatastream.h" -#include "stringmangler.h" -#include -#include "stringmangler.h" void register_SireBase_properties() { - register_property_container< SireBase::PropertyPtr, SireBase::Property >(); register_property_container< SireBase::RangePtr, SireBase::Range >(); register_property_container< SireBase::StringManglerPtr, SireBase::StringMangler >(); + register_property_container< SireBase::PropertyPtr, SireBase::Property >(); } diff --git a/wrapper/Base/SireBase_registrars.cpp b/wrapper/Base/SireBase_registrars.cpp index b3a673674..cebde76a8 100644 --- a/wrapper/Base/SireBase_registrars.cpp +++ b/wrapper/Base/SireBase_registrars.cpp @@ -3,55 +3,55 @@ #include "SireBase_registrars.h" -#include "booleanproperty.h" -#include "cpuid.h" -#include "generalunitproperty.h" +#include "propertymap.h" +#include "progressbar.h" +#include "ranges.h" +#include "variantproperty.h" #include "lengthproperty.h" -#include "linktoproperty.h" +#include "generalunitproperty.h" +#include "timeproperty.h" +#include "cpuid.h" +#include "booleanproperty.h" #include "majorminorversion.h" #include "numberproperty.h" -#include "progressbar.h" -#include "properties.h" -#include "property.h" -#include "propertylist.h" -#include "propertymap.h" -#include "ranges.h" #include "stringmangler.h" #include "stringproperty.h" -#include "timeproperty.h" -#include "variantproperty.h" +#include "linktoproperty.h" +#include "propertylist.h" +#include "property.h" +#include "properties.h" #include "Helpers/objectregistry.hpp" void register_SireBase_objects() { - ObjectRegistry::registerConverterFor< SireBase::BooleanProperty >(); - ObjectRegistry::registerConverterFor< SireBase::CPUID >(); + ObjectRegistry::registerConverterFor< SireBase::PropertyName >(); + ObjectRegistry::registerConverterFor< SireBase::PropertyMap >(); + ObjectRegistry::registerConverterFor< SireBase::ProgressBar >(); + ObjectRegistry::registerConverterFor< SireBase::SimpleRange >(); + ObjectRegistry::registerConverterFor< SireBase::VariantProperty >(); + ObjectRegistry::registerConverterFor< SireBase::LengthProperty >(); ObjectRegistry::registerConverterFor< SireBase::GeneralUnitProperty >(); ObjectRegistry::registerConverterFor< SireBase::GeneralUnitArrayProperty >(); - ObjectRegistry::registerConverterFor< SireBase::LengthProperty >(); - ObjectRegistry::registerConverterFor< SireBase::LinkToProperty >(); + ObjectRegistry::registerConverterFor< SireBase::TimeProperty >(); + ObjectRegistry::registerConverterFor< SireBase::CPUID >(); + ObjectRegistry::registerConverterFor< SireBase::BooleanProperty >(); ObjectRegistry::registerConverterFor< SireBase::MajorMinorVersion >(); ObjectRegistry::registerConverterFor< SireBase::Version >(); ObjectRegistry::registerConverterFor< SireBase::NumberProperty >(); - ObjectRegistry::registerConverterFor< SireBase::ProgressBar >(); - ObjectRegistry::registerConverterFor< SireBase::Properties >(); - ObjectRegistry::registerConverterFor< SireBase::NullProperty >(); - ObjectRegistry::registerConverterFor< SireBase::DoubleArrayProperty >(); - ObjectRegistry::registerConverterFor< SireBase::IntegerArrayProperty >(); - ObjectRegistry::registerConverterFor< SireBase::StringArrayProperty >(); - ObjectRegistry::registerConverterFor< SireBase::PropertyList >(); - ObjectRegistry::registerConverterFor< SireBase::PropertyName >(); - ObjectRegistry::registerConverterFor< SireBase::PropertyMap >(); - ObjectRegistry::registerConverterFor< SireBase::SimpleRange >(); ObjectRegistry::registerConverterFor< SireBase::NoMangling >(); ObjectRegistry::registerConverterFor< SireBase::TrimString >(); ObjectRegistry::registerConverterFor< SireBase::UpperCaseString >(); ObjectRegistry::registerConverterFor< SireBase::LowerCaseString >(); ObjectRegistry::registerConverterFor< SireBase::StringProperty >(); - ObjectRegistry::registerConverterFor< SireBase::TimeProperty >(); - ObjectRegistry::registerConverterFor< SireBase::VariantProperty >(); + ObjectRegistry::registerConverterFor< SireBase::LinkToProperty >(); + ObjectRegistry::registerConverterFor< SireBase::DoubleArrayProperty >(); + ObjectRegistry::registerConverterFor< SireBase::IntegerArrayProperty >(); + ObjectRegistry::registerConverterFor< SireBase::StringArrayProperty >(); + ObjectRegistry::registerConverterFor< SireBase::PropertyList >(); + ObjectRegistry::registerConverterFor< SireBase::NullProperty >(); + ObjectRegistry::registerConverterFor< SireBase::Properties >(); } diff --git a/wrapper/Convert/__init__.py b/wrapper/Convert/__init__.py index 21ca59e3b..88d4c7158 100644 --- a/wrapper/Convert/__init__.py +++ b/wrapper/Convert/__init__.py @@ -133,6 +133,23 @@ def sire_to_openmm(mols, map): ensemble = Ensemble(map=map) + if map.specified("cutoff"): + # we need to make sure that this is a unit + cutoff = map["cutoff"] + + if cutoff.has_source(): + cutoff = cutoff.source() + + if cutoff.lower() == "none" or cutoff.lower().startswith("infinit"): + map.set("cutoff_type", "NONE") + map.unset("cutoff") + elif cutoff.lower() == "auto": + map.unset("cutoff") + elif cutoff != "cutoff": + from ... import u + + map.set("cutoff", u(cutoff)) + if map.specified("integrator"): integrator = map["integrator"]