Make GenericParameters return optional instead of empty defaults on n…

…on-existant keys (#580)

* Add get method returning an optional value

This makes it possible to distinguish between set and empty values and
non-existant values.

* Fix unittests to use non-deprecated methods

* Remove test of deprecated type helper

* Switch to non-deprecated usage in Frame

* Make the Frame return optional parameters as well

* Bump the patch version to 99 to signify development

* Fix comments on documentation

CMakeLists.txt

@@ -8,7 +8,7 @@ project(podio)
 #--- Version -------------------------------------------------------------------
 SET( ${PROJECT_NAME}_VERSION_MAJOR 0 )
 SET( ${PROJECT_NAME}_VERSION_MINOR 99 )
-SET( ${PROJECT_NAME}_VERSION_PATCH 0 )
+SET( ${PROJECT_NAME}_VERSION_PATCH 99 )
 
 SET( ${PROJECT_NAME}_VERSION  "${${PROJECT_NAME}_VERSION_MAJOR}.${${PROJECT_NAME}_VERSION_MINOR}.${${PROJECT_NAME}_VERSION_PATCH}" )
 

include/podio/Frame.h

@@ -236,7 +236,7 @@ class Frame {
   /// @param value The value of the parameter. A copy will be put into the Frame
   template <typename T, typename = podio::EnableIfValidGenericDataType<T>>
   inline void putParameter(const std::string& key, T value) {
-    m_self->parameters().setValue(key, std::move(value));
+    m_self->parameters().set(key, std::move(value));
   }
 
   /// Add a string value to the parameters of the Frame.
@@ -278,22 +278,19 @@ class Frame {
 
   /// Retrieve parameters via key from the internal store.
   ///
-  /// The return type will either be a const reference or a value depending on
-  /// the desired type. See podio::GenericParameters for more details.
-  ///
   /// @tparam T  The desired type of the parameter (can also be std::vector<T>)
   /// @param key The key under which the value is stored
   ///
-  /// @returns   The value of the parameter or an empty default value
+  /// @returns   An optional holding the value if it is present
   template <typename T, typename = podio::EnableIfValidGenericDataType<T>>
-  inline podio::GenericDataReturnType<T> getParameter(const std::string& key) const {
-    return m_self->parameters().getValue<T>(key);
+  inline auto getParameter(const std::string& key) const {
+    return m_self->parameters().get<T>(key);
   }
 
   /// Retrieve all parameters stored in this Frame.
   ///
   /// This is mainly intended for I/O purposes and we encourage to use the Frame
-  /// functionality of getParameters or getParameterKeys in general.
+  /// functionality of getParameter or getParameterKeys in general.
   ///
   /// @returns The internally used GenericParameters
   inline const podio::GenericParameters& getParameters() const {

include/podio/GenericParameters.h

@@ -9,14 +9,17 @@
 #include <map>
 #include <memory>
 #include <mutex>
+#include <optional>
 #include <string>
 #include <vector>
 
+#if PODIO_ENABLE_SIO
 namespace sio {
 class read_device;
 class write_device;
 using version_type = uint32_t; // from sio/definitions
 } // namespace sio
+#endif
 
 #if PODIO_ENABLE_RNTUPLE
 namespace podio {
@@ -39,33 +42,6 @@ static constexpr bool isSupportedGenericDataType = detail::isAnyOrVectorOf<T, Su
 template <typename T>
 using EnableIfValidGenericDataType = typename std::enable_if_t<isSupportedGenericDataType<T>>;
 
-namespace detail {
-  /// Helper struct to determine how to return different types from the
-  /// GenericParameters to avoid unnecessary copies but also to prohibit carrying
-  /// around const references to ints or floats
-  template <typename T>
-  struct GenericDataReturnTypeHelper {
-    using type = T;
-  };
-
-  /// Specialization for std::string. Those will always be returned by const ref
-  template <>
-  struct GenericDataReturnTypeHelper<std::string> {
-    using type = const std::string&;
-  };
-
-  /// Specialization for std::vector. Those will always be returned by const ref
-  template <typename T>
-  struct GenericDataReturnTypeHelper<std::vector<T>> {
-    using type = const std::vector<T>&;
-  };
-} // namespace detail
-
-/// Alias template for determining the appropriate return type for the passed in
-/// type
-template <typename T>
-using GenericDataReturnType = typename detail::GenericDataReturnTypeHelper<T>::type;
-
 /// GenericParameters objects allow one to store generic named parameters of type
 ///  int, float and string or vectors of these types.
 ///  They can be used  to store (user) meta data that is
@@ -98,29 +74,27 @@ class GenericParameters {
 
   ~GenericParameters() = default;
 
-  /// Get the value that is stored under the given key, by const reference or by
-  /// value depending on the desired type
   template <typename T, typename = EnableIfValidGenericDataType<T>>
-  GenericDataReturnType<T> getValue(const std::string&) const;
+  std::optional<T> get(const std::string& key) const;
 
   /// Store (a copy of) the passed value under the given key
   template <typename T, typename = EnableIfValidGenericDataType<T>>
-  void setValue(const std::string& key, T value);
+  void set(const std::string& key, T value);
 
   /// Overload for catching const char* setting for string values
-  void setValue(const std::string& key, std::string value) {
-    setValue<std::string>(key, std::move(value));
+  void set(const std::string& key, std::string value) {
+    set<std::string>(key, std::move(value));
   }
 
   /// Overload for catching initializer list setting of string vector values
-  void setValue(const std::string& key, std::vector<std::string> values) {
-    setValue<std::vector<std::string>>(key, std::move(values));
+  void set(const std::string& key, std::vector<std::string> values) {
+    set<std::vector<std::string>>(key, std::move(values));
   }
 
   /// Overload for catching initializer list setting for vector values
   template <typename T, typename = std::enable_if_t<detail::isInTuple<T, SupportedGenericDataTypes>>>
-  void setValue(const std::string& key, std::initializer_list<T>&& values) {
-    setValue<std::vector<T>>(key, std::move(values));
+  void set(const std::string& key, std::initializer_list<T>&& values) {
+    set<std::vector<T>>(key, std::move(values));
   }
 
   /// Get the number of elements stored under the given key for a type
@@ -145,8 +119,10 @@ class GenericParameters {
     return _intMap.empty() && _floatMap.empty() && _stringMap.empty();
   }
 
+#if PODIO_ENABLE_SIO
   friend void writeGenericParameters(sio::write_device& device, const GenericParameters& parameters);
   friend void readGenericParameters(sio::read_device& device, GenericParameters& parameters, sio::version_type version);
+#endif
 
 #if PODIO_ENABLE_RNTUPLE
   friend RNTupleReader;
@@ -168,7 +144,7 @@ class GenericParameters {
   }
 
 private:
-  /// Get a reference to the internal map for a given type (necessary for SIO)
+  /// Get a reference to the internal map for a given type
   template <typename T>
   MapType<detail::GetVectorType<T>>& getMap() {
     if constexpr (std::is_same_v<detail::GetVectorType<T>, int>) {
@@ -182,7 +158,6 @@ class GenericParameters {
     }
   }
 
-private:
   /// Get the mutex that guards the map for the given type
   template <typename T>
   std::mutex& getMutex() const {
@@ -209,16 +184,13 @@ class GenericParameters {
 };
 
 template <typename T, typename>
-GenericDataReturnType<T> GenericParameters::getValue(const std::string& key) const {
+std::optional<T> GenericParameters::get(const std::string& key) const {
   const auto& map = getMap<T>();
   auto& mtx = getMutex<T>();
   std::lock_guard lock{mtx};
   const auto it = map.find(key);
-  // If there is no entry to the key, we just return an empty default
-  // TODO: make this case detectable from the outside
   if (it == map.end()) {
-    static const auto empty = T{};
-    return empty;
+    return std::nullopt;
   }
 
   // We have to check whether the return type is a vector or a single value
@@ -231,7 +203,7 @@ GenericDataReturnType<T> GenericParameters::getValue(const std::string& key) con
 }
 
 template <typename T, typename>
-void GenericParameters::setValue(const std::string& key, T value) {
+void GenericParameters::set(const std::string& key, T value) {
   auto& map = getMap<T>();
   auto& mtx = getMutex<T>();
 

python/podio/frame.py

@@ -1,6 +1,8 @@
 #!/usr/bin/env python3
 """Module for the python bindings of the podio::Frame"""
 
+from copy import deepcopy
+
 import cppyy
 
 import ROOT
@@ -188,10 +190,12 @@ def get_parameter(self, name, as_type=None):
         """
 
         def _get_param_value(par_type, name):
-            par_value = self._frame.getParameter[par_type](name)
+            # We can safely assume that getting the value here works, because
+            # only valid keys will end up here
+            par_value = self._frame.getParameter[par_type](name).value()
             if len(par_value) == 1:
                 return par_value[0]
-            return list(par_value)
+            return list(deepcopy(par_value))
 
         # This access already raises the KeyError if there is no such parameter
         par_type = self._param_key_types[name]

tests/read_python_frame.h

@@ -56,34 +56,34 @@ std::ostream& operator<<(std::ostream& o, const std::vector<T>& vec) {
 }
 
 int checkParameters(const podio::Frame& frame) {
-  const auto iVal = frame.getParameter<int>("an_int");
+  const auto iVal = frame.getParameter<int>("an_int").value();
   if (iVal != 42) {
     std::cerr << "Parameter an_int was not stored correctly (expected 42, actual " << iVal << ")" << std::endl;
     return 1;
   }
 
-  const auto& dVal = frame.getParameter<std::vector<double>>("some_floats");
+  const auto dVal = frame.getParameter<std::vector<double>>("some_floats").value();
   if (dVal.size() != 3 || dVal[0] != 1.23 || dVal[1] != 7.89 || dVal[2] != 3.14) {
     std::cerr << "Parameter some_floats was not stored correctly (expected [1.23, 7.89, 3.14], actual " << dVal << ")"
               << std::endl;
     return 1;
   }
 
-  const auto& strVal = frame.getParameter<std::vector<std ::string>>("greetings");
+  const auto strVal = frame.getParameter<std::vector<std ::string>>("greetings").value();
   if (strVal.size() != 2 || strVal[0] != "from" || strVal[1] != "python") {
     std::cerr << "Parameter greetings was not stored correctly (expected [from, python], actual " << strVal << ")"
               << std::endl;
     return 1;
   }
 
-  const auto realFloat = frame.getParameter<float>("real_float");
+  const auto realFloat = frame.getParameter<float>("real_float").value_or(-1.f);
   if (realFloat != 3.14f) {
     std::cerr << "Parameter real_float was not stored correctly (expected 3.14, actual " << realFloat << ")"
               << std::endl;
     return 1;
   }
 
-  const auto& realFloats = frame.getParameter<std::vector<float>>("more_real_floats");
+  const auto realFloats = frame.getParameter<std::vector<float>>("more_real_floats").value();
   if (realFloats.size() != 3 || realFloats[0] != 1.23f || realFloats[1] != 4.56f || realFloats[2] != 7.89f) {
     std::cerr << "Parameter more_real_floats was not stored as correctly (expected [1.23, 4.56, 7.89], actual"
               << realFloats << ")" << std::endl;

tests/read_test.h

@@ -39,23 +39,23 @@ bool check_fixed_width_value(FixedWidthT actual, FixedWidthT expected, const std
 }
 
 void processEvent(const podio::Frame& event, int eventNum, podio::version::Version fileVersion) {
-  const float evtWeight = event.getParameter<float>("UserEventWeight");
+  const float evtWeight = event.getParameter<float>("UserEventWeight").value();
   if (evtWeight != (float)100. * eventNum) {
     std::cout << " read UserEventWeight: " << evtWeight << " - expected : " << (float)100. * eventNum << std::endl;
     throw std::runtime_error("Couldn't read event meta data parameters 'UserEventWeight'");
   }
 
   std::stringstream ss;
   ss << " event_number_" << eventNum;
-  const auto& evtName = event.getParameter<std::string>("UserEventName");
+  const auto evtName = event.getParameter<std::string>("UserEventName").value();
 
   if (evtName != ss.str()) {
     std::cout << " read UserEventName: " << evtName << " - expected : " << ss.str() << std::endl;
     throw std::runtime_error("Couldn't read event meta data parameters 'UserEventName'");
   }
 
   if (fileVersion > podio::version::Version{0, 14, 1}) {
-    const auto& someVectorData = event.getParameter<std::vector<int>>("SomeVectorData");
+    const auto someVectorData = event.getParameter<std::vector<int>>("SomeVectorData").value();
     if (someVectorData.size() != 4) {
       throw std::runtime_error("Couldn't read event meta data parameters: 'SomeVectorData'");
     }
@@ -67,7 +67,7 @@ void processEvent(const podio::Frame& event, int eventNum, podio::version::Versi
   }
 
   if (fileVersion > podio::version::Version{0, 16, 2}) {
-    const auto& doubleParams = event.getParameter<std::vector<double>>("SomeVectorData");
+    const auto doubleParams = event.getParameter<std::vector<double>>("SomeVectorData").value();
     if (doubleParams.size() != 2 || doubleParams[0] != eventNum * 1.1 || doubleParams[1] != eventNum * 2.2) {
       throw std::runtime_error("Could not read event parameter: 'SomeDoubleValues' correctly");
     }

tests/unittests/frame.cpp

@@ -33,13 +33,13 @@ TEST_CASE("Frame parameters", "[frame][basics]") {
   REQUIRE(event.getParameter<std::string>("aString") == "from a string literal");
 
   event.putParameter("someInts", {42, 123});
-  const auto& ints = event.getParameter<std::vector<int>>("someInts");
+  const auto ints = event.getParameter<std::vector<int>>("someInts").value();
   REQUIRE(ints.size() == 2);
   REQUIRE(ints[0] == 42);
   REQUIRE(ints[1] == 123);
 
   event.putParameter("someStrings", {"one", "two", "three"});
-  const auto& strings = event.getParameter<std::vector<std::string>>("someStrings");
+  const auto strings = event.getParameter<std::vector<std::string>>("someStrings").value();
   REQUIRE(strings.size() == 3);
   REQUIRE(strings[0] == "one");
   REQUIRE(strings[1] == "two");
@@ -239,7 +239,7 @@ void checkFrame(const podio::Frame& frame) {
   REQUIRE(clusters[1].Clusters()[0] == clusters[0]);
 
   REQUIRE(frame.getParameter<int>("anInt") == 42);
-  auto& floats = frame.getParameter<std::vector<float>>("someFloats");
+  const auto floats = frame.getParameter<std::vector<float>>("someFloats").value();
   REQUIRE(floats.size() == 3);
   REQUIRE(floats[0] == 1.23f);
   REQUIRE(floats[1] == 2.34f);
@@ -339,7 +339,7 @@ TEST_CASE("Frame parameters multithread insert and read", "[frame][basics][multi
       frame.putParameter(makeName("string", i), std::to_string(i));
       CHECK_INCREASE(frame.getParameter<std::string>("string_par") == "some string", successes[i]);
 
-      const auto& floatPars = frame.getParameter<std::vector<float>>("float_pars");
+      const auto floatPars = frame.getParameter<std::vector<float>>("float_pars").value();
       CHECK_INCREASE(floatPars.size() == 3, successes[i]);
       CHECK_INCREASE(floatPars[0] == 1.23f, successes[i]);
       CHECK_INCREASE(floatPars[1] == 4.56f, successes[i]);