[RF][HS3] Clean roundtripping of RooExponential and RooLogNormal

Make sure that when transforming workspaces with these classes,
roundtripping from RooFit -> JSON -> RooFit -> JSON leaves both the
RooFit workspace and the JSON unchanged.

Closes #15756.

Author: guitargeek

roofit/hs3/inc/RooFitHS3/RooJSONFactoryWSTool.h

@@ -181,11 +181,7 @@ class RooJSONFactoryWSTool {
 
    void queueExport(RooAbsArg const &arg) { _serversToExport.push_back(&arg); }
 
-   RooFit::Detail::JSONNode &createAdHoc(const std::string &toplevel, const std::string &name);
-   RooAbsReal *importTransformed(const std::string &name, const std::string &tag, const std::string &operation_name,
-                                 const std::string &formula);
-   std::string exportTransformed(const RooAbsReal *original, const std::string &tag, const std::string &operation_name,
-                                 const std::string &formula);
+   std::string exportTransformed(const RooAbsReal *original, const std::string &suffix, const std::string &formula);
 
    void setAttribute(const std::string &obj, const std::string &attrib);
    bool hasAttribute(const std::string &obj, const std::string &attrib);

roofit/hs3/src/JSONFactories_RooFitCore.cxx

@@ -35,6 +35,8 @@
 #include <RooTFnBinding.h>
 #include <RooWorkspace.h>
 
+#include "JSONIOUtils.h"
+
 #include <TF1.h>
 #include <TH1.h>
 
@@ -237,12 +239,18 @@ class RooLogNormalFactory : public RooFit::JSONIO::Importer {
    {
       std::string name(RooJSONFactoryWSTool::name(p));
       RooAbsReal *x = tool->requestArg<RooAbsReal>(p, "x");
-      RooAbsReal *mu = tool->requestArg<RooAbsReal>(p, "mu");
-      RooAbsReal *sigma = tool->requestArg<RooAbsReal>(p, "sigma");
 
-      // TODO: check if the pdf was originally exported by ROOT, in which case
-      // it can be imported back without using the standard parametrization.
-      tool->wsEmplace<RooLognormal>(name, *x, *mu, *sigma, true);
+      // Same mechanism to undo the parameter transformation as in the
+      // RooExponentialFactory (see comments in that class for more info).
+      const std::string muName = p["mu"].val();
+      const std::string sigmaName = p["sigma"].val();
+      const bool isTransformed = endsWith(muName, "_lognormal_log");
+      const std::string suffixToRemove = isTransformed ? "_lognormal_log" : "";
+      RooAbsReal *mu = tool->request<RooAbsReal>(removeSuffix(muName, suffixToRemove), name);
+      RooAbsReal *sigma = tool->request<RooAbsReal>(removeSuffix(sigmaName, suffixToRemove), name);
+
+      tool->wsEmplace<RooLognormal>(name, *x, *mu, *sigma, !isTransformed);
+
       return true;
    }
 };
@@ -253,11 +261,45 @@ class RooExponentialFactory : public RooFit::JSONIO::Importer {
    {
       std::string name(RooJSONFactoryWSTool::name(p));
       RooAbsReal *x = tool->requestArg<RooAbsReal>(p, "x");
-      RooAbsReal *c = tool->requestArg<RooAbsReal>(p, "c");
 
-      // TODO: check if the pdf was originally exported by ROOT, in which case
-      // it can be imported back without using the standard parametrization.
-      tool->wsEmplace<RooExponential>(name, *x, *c, true);
+      // If the parameter name ends with the "_exponential_inverted" suffix,
+      // this means that it was exported from a RooFit object where the
+      // parameter first needed to be transformed on export to match the HS3
+      // specification. But when re-importing such a parameter, we can simply
+      // skip the transformation and use the original RooFit parameter without
+      // the suffix.
+      //
+      // A concrete example: take the following RooFit pdf in the factory language:
+      //
+      //    "Exponential::exponential_1(x[0, 10], c[-0.1])"
+      //
+      //  It defines en exponential exp(c * x). However, in HS3 the exponential
+      //  is defined as exp(-c * x), to RooFit would export these dictionaries
+      //  to the JSON:
+      //
+      //  {
+      //      "name": "exponential_1",             // HS3 exponential_dist with transformed parameter
+      //      "type": "exponential_dist",
+      //      "x": "x",
+      //      "c": "c_exponential_inverted"
+      //  },
+      //  {
+      //      "name": "c_exponential_inverted",    // transformation function created on-the-fly on export
+      //      "type": "generic_function",
+      //      "expression": "-c"
+      //  }
+      //
+      //  On import, we can directly take the non-transformed parameter, which is
+      //  we check for the suffix and optionally remove it from the requested
+      //  name next:
+
+      const std::string constParamName = p["c"].val();
+      const bool isInverted = endsWith(constParamName, "_exponential_inverted");
+      const std::string suffixToRemove = isInverted ? "_exponential_inverted" : "";
+      RooAbsReal *c = tool->request<RooAbsReal>(removeSuffix(constParamName, suffixToRemove), name);
+
+      tool->wsEmplace<RooExponential>(name, *x, *c, !isInverted);
+
       return true;
    }
 };
@@ -562,12 +604,12 @@ class RooLogNormalStreamer : public RooFit::JSONIO::Exporter {
       auto &m0 = pdf->getMedian();
       auto &k = pdf->getShapeK();
 
-      if(pdf->useStandardParametrization()) {
+      if (pdf->useStandardParametrization()) {
          elem["mu"] << m0.GetName();
          elem["sigma"] << k.GetName();
       } else {
-         elem["mu"] << tool->exportTransformed(&m0, "lognormal", "log", "log(%s)");
-         elem["sigma"] << tool->exportTransformed(&k, "lognormal", "log", "log(%s)");
+         elem["mu"] << tool->exportTransformed(&m0, "_lognormal_log", "log(%s)");
+         elem["sigma"] << tool->exportTransformed(&k, "_lognormal_log", "log(%s)");
       }
 
       return true;
@@ -586,7 +628,7 @@ class RooExponentialStreamer : public RooFit::JSONIO::Exporter {
       if (pdf->negateCoefficient()) {
          elem["c"] << c.GetName();
       } else {
-         elem["c"] << tool->exportTransformed(&c, "exponential", "inverted", "-%s");
+         elem["c"] << tool->exportTransformed(&c, "_exponential_inverted", "-%s");
       }
 
       return true;

roofit/hs3/src/JSONIOUtils.cxx

@@ -1,5 +1,7 @@
 #include "JSONIOUtils.h"
 
+#include <string>
+
 using RooFit::Detail::JSONNode;
 using RooFit::Detail::JSONTree;
 
@@ -13,6 +15,21 @@ bool endsWith(std::string_view str, std::string_view suffix)
    return str.size() >= suffix.size() && 0 == str.compare(str.size() - suffix.size(), suffix.size(), suffix);
 }
 
+std::string removePrefix(std::string_view str, std::string_view prefix)
+{
+   std::string out;
+   out += str;
+   out = out.substr(prefix.length());
+   return out;
+}
+std::string removeSuffix(std::string_view str, std::string_view suffix)
+{
+   std::string out;
+   out += str;
+   out = out.substr(0, out.length() - suffix.length());
+   return out;
+}
+
 std::unique_ptr<RooFit::Detail::JSONTree> varJSONString(const JSONNode &treeRoot)
 {
    std::string varName = treeRoot.find("name")->val();
@@ -71,4 +88,4 @@ std::unique_ptr<RooFit::Detail::JSONTree> varJSONString(const JSONNode &treeRoot
    }
 
    return jsonDict;
-}
+}

roofit/hs3/src/JSONIOUtils.h

@@ -6,6 +6,8 @@
 
 bool startsWith(std::string_view str, std::string_view prefix);
 bool endsWith(std::string_view str, std::string_view suffix);
+std::string removePrefix(std::string_view str, std::string_view prefix);
+std::string removeSuffix(std::string_view str, std::string_view suffix);
 std::unique_ptr<RooFit::Detail::JSONTree> varJSONString(const RooFit::Detail::JSONNode &treeRoot);
 
 #endif

roofit/hs3/src/RooJSONFactoryWSTool.cxx

@@ -956,57 +956,17 @@ void RooJSONFactoryWSTool::exportVariables(const RooArgSet &allElems, JSONNode &
    }
 }
 
-RooAbsReal *RooJSONFactoryWSTool::importTransformed(const std::string &name, const std::string &tag,
-                                                    const std::string &operation_name, const std::string &formula)
+std::string RooJSONFactoryWSTool::exportTransformed(const RooAbsReal *original, const std::string &suffix,
+                                                    const std::string &formula)
 {
-   RooAbsReal *transformed = nullptr;
-   const std::string tagname = "autogen_transform_" + tag;
-   if (this->hasAttribute(name, tagname)) {
-      const std::string &original = this->getStringAttribute(name, tagname + "_original");
-      transformed = this->workspace()->function(original);
-      if (transformed)
-         return transformed;
-   }
-   const std::string newname = name + "_" + tag + "_" + operation_name;
-   transformed = this->workspace()->function(newname);
-   if (!transformed) {
-      auto *original = this->workspace()->arg(name);
-      if (!original) {
-         error("unable to import transformed of '" + name + "', original not present.");
-      }
-      RooArgSet components{*original};
-      const std::string &expression = TString::Format(formula.c_str(), name.c_str()).Data();
-      transformed = &wsEmplace<RooFormulaVar>(newname, expression.c_str(), components);
-      transformed->setAttribute(tagname.c_str());
-   }
-   return transformed;
-}
-
-std::string RooJSONFactoryWSTool::exportTransformed(const RooAbsReal *original, const std::string &tag,
-                                                    const std::string &operation_name, const std::string &formula)
-{
-   const std::string tagname = "autogen_transform_" + tag;
-   if (original->getAttribute(tagname.c_str())) {
-      if (const RooFormulaVar *trafo = dynamic_cast<const RooFormulaVar *>(original)) {
-         return trafo->dependents().first()->GetName();
-      }
-   }
-
-   std::string newname = std::string(original->GetName()) + "_" + tag + "_" + operation_name;
-   auto &trafo_node = this->createAdHoc("functions", newname);
+   std::string newname = std::string(original->GetName()) + suffix;
+   RooFit::Detail::JSONNode &trafo_node = appendNamedChild((*_rootnodeOutput)["functions"], newname);
    trafo_node["type"] << "generic_function";
    trafo_node["expression"] << TString::Format(formula.c_str(), original->GetName()).Data();
-   this->setAttribute(newname, tagname);
-   this->setStringAttribute(newname, tagname + "_original", original->GetName());
+   this->setAttribute(newname, "roofit_skip"); // this function should not be imported back in
    return newname;
 }
 
-RooFit::Detail::JSONNode &RooJSONFactoryWSTool::createAdHoc(const std::string &toplevel, const std::string &name)
-{
-   auto &collectionNode = (*_rootnodeOutput)[toplevel];
-   return appendNamedChild(collectionNode, name);
-}
-
 /**
  * @brief Export an object from the workspace to a JSONNode.
  *
@@ -1167,11 +1127,17 @@ void RooJSONFactoryWSTool::exportObject(RooAbsArg const &func, std::set<std::str
  */
 void RooJSONFactoryWSTool::importFunction(const JSONNode &p, bool importAllDependants)
 {
+   std::string name(RooJSONFactoryWSTool::name(p));
+
+   // If this node if marked to be skipped by RooFit, exit
+   if (hasAttribute(name, "roofit_skip")) {
+      return;
+   }
+
    auto const &importers = RooFit::JSONIO::importers();
    auto const &factoryExpressions = RooFit::JSONIO::importExpressions();
 
    // some preparations: what type of function are we dealing with here?
-   std::string name(RooJSONFactoryWSTool::name(p));
    if (!::isValidName(name)) {
       std::stringstream ss;
       ss << "RooJSONFactoryWSTool() function name '" << name << "' is not valid!" << std::endl;