Support scoped key-value tree transform rules
authorTeemu Murtola <teemu.murtola@gmail.com>
Sun, 9 Jul 2017 06:28:26 +0000 (09:28 +0300)
committerMark Abraham <mark.j.abraham@gmail.com>
Tue, 11 Jul 2017 08:23:23 +0000 (10:23 +0200)
Resolve several TODOs about having the knowledge of the "applied-forces"
option section scattered in different places.  It is now possible to
create a "scoped" key-value tree transform rule interface such that all
rules created through it target a specific subtree in the target tree.

This makes it possible to have initMdpOptions() and initMdpTransform()
operate on the same tree in electricfield.cpp, and have the higher-level
structure only declared in mdmodules.cpp.  Remove the dependency on
MDModules from electric field tests, since it can now use these two
methods without knowing anything about the presence of the higher-level
"applied-forces" section.

Change-Id: I8107b10af3c9c602f40297a2279e3b0449e27e8d

src/gromacs/applied-forces/electricfield.cpp
src/gromacs/applied-forces/tests/electricfield.cpp
src/gromacs/mdrunutility/mdmodules.cpp
src/gromacs/options/treesupport.cpp
src/gromacs/utility/keyvaluetree.h
src/gromacs/utility/keyvaluetreetransform.cpp
src/gromacs/utility/keyvaluetreetransform.h
src/gromacs/utility/tests/keyvaluetreetransform.cpp
src/gromacs/utility/tests/refdata/TreeValueTransformTest_ScopedTransformRules.xml [new file with mode: 0644]

index 40da0f1a31e7302cdcf9efe9c2027bf8fa7947a6..e0d8dfa918e7fcd8a8fb0b27c58d5c224bf13e13 100644 (file)
@@ -302,21 +302,17 @@ void convertDynamicParameters(gmx::KeyValueTreeObjectBuilder *builder,
 
 void ElectricField::initMdpTransform(IKeyValueTreeTransformRules *rules)
 {
-    // TODO This responsibility should be handled by the caller,
-    // e.g. embedded in the rules, somehow.
-    std::string prefix = "/applied-forces/";
-
-    rules->addRule().from<std::string>("/E-x").to<real>(prefix + "electric-field/x/E0")
+    rules->addRule().from<std::string>("/E-x").to<real>("/electric-field/x/E0")
         .transformWith(&convertStaticParameters);
-    rules->addRule().from<std::string>("/E-xt").toObject(prefix + "electric-field/x")
+    rules->addRule().from<std::string>("/E-xt").toObject("/electric-field/x")
         .transformWith(&convertDynamicParameters);
-    rules->addRule().from<std::string>("/E-y").to<real>(prefix + "electric-field/y/E0")
+    rules->addRule().from<std::string>("/E-y").to<real>("/electric-field/y/E0")
         .transformWith(&convertStaticParameters);
-    rules->addRule().from<std::string>("/E-yt").toObject(prefix + "electric-field/y")
+    rules->addRule().from<std::string>("/E-yt").toObject("/electric-field/y")
         .transformWith(&convertDynamicParameters);
-    rules->addRule().from<std::string>("/E-z").to<real>(prefix + "electric-field/z/E0")
+    rules->addRule().from<std::string>("/E-z").to<real>("/electric-field/z/E0")
         .transformWith(&convertStaticParameters);
-    rules->addRule().from<std::string>("/E-zt").toObject(prefix + "electric-field/z")
+    rules->addRule().from<std::string>("/E-zt").toObject("/electric-field/z")
         .transformWith(&convertDynamicParameters);
 }
 
index d621f9012721b4e857a19cdab6961760ea8dc1c4..b2ab943c9e4c8ccf665f0d871efc9b761f4037bb 100644 (file)
 #include "gromacs/gmxlib/network.h"
 #include "gromacs/math/vec.h"
 #include "gromacs/mdlib/forcerec.h"
-#include "gromacs/mdrunutility/mdmodules.h"
 #include "gromacs/mdtypes/forcerec.h"
 #include "gromacs/mdtypes/iforceprovider.h"
+#include "gromacs/mdtypes/imdmodule.h"
+#include "gromacs/mdtypes/imdpoptionprovider.h"
 #include "gromacs/mdtypes/inputrec.h"
 #include "gromacs/mdtypes/mdatom.h"
+#include "gromacs/options/options.h"
+#include "gromacs/options/treesupport.h"
 #include "gromacs/utility/arrayref.h"
 #include "gromacs/utility/keyvaluetreebuilder.h"
 #include "gromacs/utility/keyvaluetreetransform.h"
@@ -82,11 +85,11 @@ class ElectricFieldTest : public ::testing::Test
         {
             gmx::test::FloatingPointTolerance tolerance(
                     gmx::test::relativeToleranceAsFloatingPoint(1.0, 0.005));
-            gmx::MDModules                    module;
+            auto                              module(gmx::createElectricFieldModule());
 
             // Prepare MDP inputs
             const char *dimXYZ[3] = { "x", "y", "z" };
-            GMX_RELEASE_ASSERT((dim >= 0 && dim < 3), "Dimension should be 0, 1 or 2");
+            GMX_RELEASE_ASSERT(dim >= 0 && dim < DIM, "Dimension should be 0, 1 or 2");
 
             gmx::KeyValueTreeBuilder     mdpValues;
             mdpValues.rootObject().addValue(gmx::formatString("E%s", dimXYZ[dim]),
@@ -97,9 +100,14 @@ class ElectricFieldTest : public ::testing::Test
             gmx::KeyValueTreeTransformer transform;
             transform.rules()->addRule()
                 .keyMatchType("/", gmx::StringCompareType::CaseAndDashInsensitive);
-            module.initMdpTransform(transform.rules());
-            auto result = transform.transform(mdpValues.build(), nullptr);
-            module.assignOptionsToModules(result.object(), nullptr);
+            module->mdpOptionProvider()->initMdpTransform(transform.rules());
+            auto         result = transform.transform(mdpValues.build(), nullptr);
+            gmx::Options moduleOptions;
+            module->mdpOptionProvider()->initMdpOptions(&moduleOptions);
+            gmx::assignOptionsFromKeyValueTree(&moduleOptions, result.object(), nullptr);
+
+            ForceProviders forceProviders;
+            module->initForceProviders(&forceProviders);
 
             t_mdatoms        md;
             PaddedRVecVector f = { { 0, 0, 0 } };
@@ -108,9 +116,10 @@ class ElectricFieldTest : public ::testing::Test
             md.chargeA[0] = 1;
 
             t_commrec  *cr = init_commrec();
-            module.initForceProviders()->calculateForces(cr, &md, nullptr, 0, nullptr,
-                                                         gmx::EmptyArrayRef(), f);
+            forceProviders.calculateForces(cr, &md, nullptr, 0, nullptr,
+                                           gmx::EmptyArrayRef(), f);
             done_commrec(cr);
+
             EXPECT_REAL_EQ_TOL(f[0][dim], expectedValue, tolerance);
             sfree(md.chargeA);
         }
index 02cb435225f24c213b627f3f8c278efcf7bbbab3..c74726b9e5a1e5ec0a41fd24ed47a755d6b11e66 100644 (file)
@@ -48,6 +48,7 @@
 #include "gromacs/options/optionsection.h"
 #include "gromacs/options/treesupport.h"
 #include "gromacs/utility/keyvaluetree.h"
+#include "gromacs/utility/keyvaluetreetransform.h"
 #include "gromacs/utility/smalloc.h"
 
 namespace gmx
@@ -95,10 +96,8 @@ MDModules::~MDModules()
 
 void MDModules::initMdpTransform(IKeyValueTreeTransformRules *rules)
 {
-    // TODO The transform rules for applied-forces modules should
-    // embed the necessary prefix (and similarly for other groupings
-    // of modules). For now, electric-field embeds this itself.
-    impl_->field_->mdpOptionProvider()->initMdpTransform(rules);
+    auto appliedForcesScope = rules->scopedTransform("/applied-forces");
+    impl_->field_->mdpOptionProvider()->initMdpTransform(appliedForcesScope.rules());
 }
 
 void MDModules::assignOptionsToModules(const KeyValueTreeObject  &params,
index 215d672be8d7d38973655d347fac11427b1517cd..9edaaa8a5b1d25b3d1d4731afa008e1d7aa40e87 100644 (file)
@@ -173,9 +173,7 @@ class TreeCheckHelper : private OptionsVisitor
             {
                 if (knownNames.count(prop.key()) == 0)
                 {
-                    KeyValueTreePath path(currentPath_);
-                    path.append(prop.key());
-                    unknownPaths_.push_back(std::move(path));
+                    unknownPaths_.push_back(currentPath_ + prop.key());
                 }
             }
         }
index 61f4bdab41a277aab38fa8c563e6b0290da29dac..5b7ea96c088d335289ed1dde13c88d6c76a52059 100644 (file)
@@ -113,6 +113,12 @@ class KeyValueTreePath
 
         //! Adds another element to the path, making it a child of the old path.
         void append(const std::string &key) { path_.push_back(key); }
+        //! Adds elements from another path to the path.
+        void append(const KeyValueTreePath &other)
+        {
+            auto elements = other.elements();
+            path_.insert(path_.end(), elements.begin(), elements.end());
+        }
         //! Removes the last element in the path, making it the parent path.
         void pop_back() { return path_.pop_back(); }
         //! Removes and returns the last element in the path.
@@ -139,6 +145,25 @@ class KeyValueTreePath
         std::vector<std::string> path_;
 };
 
+//! \cond libapi
+
+//! Combines two paths as with KeyValueTreePath::append().
+inline KeyValueTreePath operator+(const KeyValueTreePath &a, const KeyValueTreePath &b)
+{
+    KeyValueTreePath result(a);
+    result.append(b);
+    return result;
+}
+
+//! Combines an element to a path as with KeyValueTreePath::append().
+inline KeyValueTreePath operator+(const KeyValueTreePath &a, const std::string &b)
+{
+    KeyValueTreePath result(a);
+    result.append(b);
+    return result;
+}
+//! \endcond
+
 class KeyValueTreeValue
 {
     public:
index 409bf0189c8cab6c7b03c790b40fd3a3c2924840..7d861157dd7338f358bbdcb40ddf6d00a1d1ae7b 100644 (file)
@@ -59,6 +59,60 @@ IKeyValueTreeTransformRules::~IKeyValueTreeTransformRules()
 {
 }
 
+/********************************************************************
+ * KeyValueTreeTransformRulesScoped::Impl
+ */
+
+class KeyValueTreeTransformRulesScoped::Impl : public IKeyValueTreeTransformRules
+{
+    public:
+        Impl(internal::KeyValueTreeTransformerImpl *impl, const KeyValueTreePath &prefix)
+            : impl_(impl), prefix_(prefix)
+        {
+        }
+
+        KeyValueTreeTransformRuleBuilder addRule() override
+        {
+            return KeyValueTreeTransformRuleBuilder(impl_, prefix_);
+        }
+
+        KeyValueTreeTransformRulesScoped
+        scopedTransform(const KeyValueTreePath &scope) override
+        {
+            return KeyValueTreeTransformRulesScoped(impl_, prefix_ + scope);
+        }
+
+    private:
+        internal::KeyValueTreeTransformerImpl *impl_;
+        KeyValueTreePath                       prefix_;
+};
+
+/********************************************************************
+ * KeyValueTreeTransformRulesScoped
+ */
+
+KeyValueTreeTransformRulesScoped::KeyValueTreeTransformRulesScoped(
+        internal::KeyValueTreeTransformerImpl *impl, const KeyValueTreePath &prefix)
+    : impl_(new Impl(impl, prefix))
+{
+}
+
+KeyValueTreeTransformRulesScoped::KeyValueTreeTransformRulesScoped(
+        KeyValueTreeTransformRulesScoped &&) = default;
+
+KeyValueTreeTransformRulesScoped &
+KeyValueTreeTransformRulesScoped::operator=(
+        KeyValueTreeTransformRulesScoped &&) = default;
+
+KeyValueTreeTransformRulesScoped::~KeyValueTreeTransformRulesScoped()
+{
+}
+
+IKeyValueTreeTransformRules *KeyValueTreeTransformRulesScoped::rules()
+{
+    return impl_.get();
+}
+
 /********************************************************************
  * IKeyValueTreeBackMapping
  */
@@ -147,7 +201,7 @@ namespace internal
  * KeyValueTreeTransformerImpl
  */
 
-class KeyValueTreeTransformerImpl : public IKeyValueTreeTransformRules
+class KeyValueTreeTransformerImpl
 {
     public:
         class Rule
@@ -253,9 +307,9 @@ class KeyValueTreeTransformerImpl : public IKeyValueTreeTransformRules
                 KeyValueTreePath                         context_;
         };
 
-        virtual KeyValueTreeTransformRuleBuilder addRule()
+        KeyValueTreeTransformerImpl()
+            : rootScope_(this, KeyValueTreePath())
         {
-            return KeyValueTreeTransformRuleBuilder(this);
         }
 
         Rule *getOrCreateRootRule()
@@ -273,7 +327,8 @@ class KeyValueTreeTransformerImpl : public IKeyValueTreeTransformRules
             rootRule_.reset(new Rule(keyMatchType));
         }
 
-        std::unique_ptr<Rule>  rootRule_;
+        std::unique_ptr<Rule>             rootRule_;
+        KeyValueTreeTransformRulesScoped  rootScope_;
 };
 
 /********************************************************************
@@ -382,7 +437,7 @@ KeyValueTreeTransformer::~KeyValueTreeTransformer()
 
 IKeyValueTreeTransformRules *KeyValueTreeTransformer::rules()
 {
-    return impl_.get();
+    return impl_->rootScope_.rules();
 }
 
 std::vector<KeyValueTreePath> KeyValueTreeTransformer::mappedPaths() const
@@ -413,8 +468,8 @@ class KeyValueTreeTransformRuleBuilder::Data
     public:
         typedef internal::KeyValueTreeTransformerImpl::Rule Rule;
 
-        Data()
-            : expectedType_(typeid(void)),
+        explicit Data(const KeyValueTreePath &prefix)
+            : prefixPath_(prefix), expectedType_(typeid(void)),
               keyMatchType_(StringCompareType::Exact), keyMatchRule_(false)
         {
         }
@@ -461,6 +516,7 @@ class KeyValueTreeTransformRuleBuilder::Data
             }
         }
 
+        const KeyValueTreePath   prefixPath_;
         KeyValueTreePath         fromPath_;
         KeyValueTreePath         toPath_;
         std::type_index          expectedType_;
@@ -474,8 +530,8 @@ class KeyValueTreeTransformRuleBuilder::Data
  */
 
 KeyValueTreeTransformRuleBuilder::KeyValueTreeTransformRuleBuilder(
-        internal::KeyValueTreeTransformerImpl *impl)
-    : impl_(impl), data_(new Data)
+        internal::KeyValueTreeTransformerImpl *impl, const KeyValueTreePath &prefix)
+    : impl_(impl), data_(new Data(prefix))
 {
 }
 
@@ -499,7 +555,7 @@ void KeyValueTreeTransformRuleBuilder::setExpectedType(const std::type_index &ty
 
 void KeyValueTreeTransformRuleBuilder::setToPath(const KeyValueTreePath &path)
 {
-    data_->toPath_ = path;
+    data_->toPath_ = data_->prefixPath_ + path;
 }
 
 void KeyValueTreeTransformRuleBuilder::setKeyMatchType(StringCompareType keyMatchType)
index 2c584609ed95a4377c604b89f8075fbe11e773de..a44a65830b9dfb8a250b5d3015c12a7379029514 100644 (file)
@@ -64,6 +64,7 @@ enum class StringCompareType;
 
 class KeyValueTreeTransformResult;
 class KeyValueTreeTransformRuleBuilder;
+class KeyValueTreeTransformRulesScoped;
 
 namespace internal
 {
@@ -97,11 +98,50 @@ class IKeyValueTreeTransformRules
          * builder.
          */
         virtual KeyValueTreeTransformRuleBuilder addRule() = 0;
+        /*! \brief
+         * Creates a scoped set of rules, where all rules use a target sub-tree.
+         *
+         * \param[in] scope Prefix defining the scope in the target tree
+         *
+         * Any rules added to the returned scope will have `scope` prefixed to
+         * their target paths, i.e., it is not possible to produce elements
+         * outside the specified subtree.
+         */
+        virtual KeyValueTreeTransformRulesScoped
+        scopedTransform(const KeyValueTreePath &scope) = 0;
 
     protected:
         ~IKeyValueTreeTransformRules();
 };
 
+/*! \libinternal \brief
+ * Helper object returned from IKeyValueTreeTransformRules::scopedTransform().
+ *
+ * \inlibraryapi
+ * \ingroup module_utility
+ */
+class KeyValueTreeTransformRulesScoped
+{
+    public:
+        //! Internal constructor for creating the scope.
+        KeyValueTreeTransformRulesScoped(
+            internal::KeyValueTreeTransformerImpl *impl,
+            const KeyValueTreePath                &prefix);
+        //! Supports returning the object from IKeyValueTreeTransformRules::scopedTransform().
+        KeyValueTreeTransformRulesScoped(KeyValueTreeTransformRulesScoped &&other);
+        //! Supports returning the object from IKeyValueTreeTransformRules::scopedTransform().
+        KeyValueTreeTransformRulesScoped &operator=(KeyValueTreeTransformRulesScoped &&other);
+        ~KeyValueTreeTransformRulesScoped();
+
+        //! Returns the interface for adding rules to this scope.
+        IKeyValueTreeTransformRules *rules();
+
+    private:
+        class Impl;
+
+        PrivateImplPointer<Impl> impl_;
+};
+
 /*! \libinternal \brief
  * Provides methods to specify one transformation rule.
  *
@@ -255,7 +295,8 @@ class KeyValueTreeTransformRuleBuilder
         };
 
         //! Internal constructor for creating a builder.
-        explicit KeyValueTreeTransformRuleBuilder(internal::KeyValueTreeTransformerImpl *impl);
+        KeyValueTreeTransformRuleBuilder(internal::KeyValueTreeTransformerImpl *impl,
+                                         const KeyValueTreePath                &prefix);
         //! Supports returning the builder from IKeyValueTreeTransformRules::addRule().
         KeyValueTreeTransformRuleBuilder(KeyValueTreeTransformRuleBuilder &&)            = default;
         //! Supports returning the builder from IKeyValueTreeTransformRules::addRule().
index 64ba3160f04fadd263ceef365ef030a1529f1395..c512cf6382bec09e8f43721d29f2e00047b1e409 100644 (file)
@@ -1,7 +1,7 @@
 /*
  * This file is part of the GROMACS molecular simulation package.
  *
- * Copyright (c) 2016, by the GROMACS development team, led by
+ * Copyright (c) 2016,2017, by the GROMACS development team, led by
  * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl,
  * and including many others, as listed in the AUTHORS file in the
  * top-level source directory and at http://www.gromacs.org.
@@ -201,6 +201,23 @@ TEST_F(TreeValueTransformTest, ObjectFromMultipleStrings)
     testTransform(input, transform);
 }
 
+TEST_F(TreeValueTransformTest, ScopedTransformRules)
+{
+    gmx::KeyValueTreeBuilder     builder;
+    builder.rootObject().addValue<std::string>("a", "1");
+    builder.rootObject().addValue<std::string>("b", "2");
+    gmx::KeyValueTreeObject      input = builder.build();
+
+    gmx::KeyValueTreeTransformer transform;
+    auto                         scope = transform.rules()->scopedTransform("/foo");
+    scope.rules()->addRule()
+        .from<std::string>("/a").to<int>("/i").transformWith(&gmx::fromStdString<int>);
+    scope.rules()->addRule()
+        .from<std::string>("/b").to<int>("/j").transformWith(&gmx::fromStdString<int>);
+
+    testTransform(input, transform);
+}
+
 /********************************************************************
  * Tests for errors
  */
diff --git a/src/gromacs/utility/tests/refdata/TreeValueTransformTest_ScopedTransformRules.xml b/src/gromacs/utility/tests/refdata/TreeValueTransformTest_ScopedTransformRules.xml
new file mode 100644 (file)
index 0000000..603fd2b
--- /dev/null
@@ -0,0 +1,23 @@
+<?xml version="1.0"?>
+<?xml-stylesheet type="text/xsl" href="referencedata.xsl"?>
+<ReferenceData>
+  <Object Name="Input">
+    <String Name="a">1</String>
+    <String Name="b">2</String>
+  </Object>
+  <Sequence Name="MappedPaths">
+    <Int Name="Length">2</Int>
+    <String>/a</String>
+    <String>/b</String>
+  </Sequence>
+  <Object Name="Tree">
+    <Object Name="foo">
+      <Int Name="i">1</Int>
+      <Int Name="j">2</Int>
+    </Object>
+  </Object>
+  <BackMapping Name="Mapping">
+    <String Name="/foo/i">/a</String>
+    <String Name="/foo/j">/b</String>
+  </BackMapping>
+</ReferenceData>