Optional input variables with default value

[IzaakWN]

Related to Issues #2 and #5, it would be useful to have a default value for some input variables, e.g.

  'inputs': [
      {'name': "eta",      'type': "real" },
      {'name': "pt",       'type': "real" },
      {'name': "genmatch", 'type': "int", 'default': 5 },
      {'name': "syst",     'type': "string", 'default': "nom"},
  ],

[nsmith-]

What would the evaluator function signature be in this case? For python, all values with a default would have to be at the end, and one might expect kwargs support. For C++ I'm not sure what it would look like. We already have a bit of a hack with the initializer-list because variadic function templates make my head hurt.

[pieterdavid]

The evaluator could provide a getter method for the default values, and an alternative (less efficient) evaluate method that takes a map (e.g. boost::container::flat_map in C++) or only keyword arguments (python) and fills the defaults - that may be useful to change scalefactors with minimal code changes, but I think I'd still prefer the version without defaults in most cases.

[nsmith-]

This is reasonable, though I think std::map would work fine (we don't need an ordering in the container since the correction inputs define an order). Here's what that might look like:

double out = cset["scalefactors_Tight_Electron"].evaluate({ {"pt", 35.}, {"eta", 1.3} });

with the associated patch

diff --git a/include/correction.h b/include/correction.h
index 31a7f25..f5daba8 100644
--- a/include/correction.h
+++ b/include/correction.h
@@ -84,6 +84,18 @@ class Correction {
     Correction(const rapidjson::Value& json);
     std::string name() const { return name_; };
     double evaluate(const std::vector<Variable::Type>& values) const;
+    double evaluate(const std::map<std::string, Variable::Type>& values) const {
+      std::vector<Variable::Type> varargs;
+      for (auto var : inputs_) {
+        try {
+          varargs.push_back(values.at(var.name()));
+        }
+        catch ( std::out_of_range ) {
+          varargs.push_back(0.); // here would be a default token
+        }
+      }
+      return evaluate(varargs);
+    };

I'm not sure if overload is the right approach though, might be better to have a separate evaluate_kw

[nsmith-]

Defaults for Category are now handled via #33, though it was pointed out there should be an "explicit default" so users do not have to make up a dummy value to access the default. For binning nodes, it isn't really clear to me the use case for a default floating-point value. But it could be accommodated in the evaluate() method using the sentinel approach I proposed in https://github.com/nsmith-/correctionlib/pull/33#issuecomment-780620356

Regardless, a keyword evaluate method should be added. Also, since part of the motivation for having default values is to reduce redundant user code, that could also be accomplished by providing partially evaluated corrections, tracked in #38.

[nsmith-]

I realized that input variables are implicitly optional if no nodes appear in the tree walk that depend on the input, in addition to the case where they are explicitly optional due to being string or integer enumerations on a Category that has a default specified. But one cannot know that ahead of time, only on evaluation. So it seems to me the cleanest implementation is to allow users to specify an "explicit default" and raise an exception on evaluation if the variable was required and not provided. This can be accomplished via a special object in the variant supplied to the evaluate call, e.g.:

// corr takes two real-valued inputs, the second of which is not used in the case that the first is == 2.
double result = corr.evaluate({2., Variable::default});

where Variable::default is a static member that is an instance of std::monostate which we add to the Variable::Type variant. In python, we might consider letting None have the same meaning as Variable::default.

The description of the input could make a note whether there is possibility of a variable being optional. Real-valued inputs could only be conditionally optional based on other inputs, since otherwise one can just omit the binning node for that variable in the content tree. Hence, I don't think any schema change is needed for this feature.