Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make GenericParameters return optional instead of empty defaults on non-existant keys #580

Merged
merged 11 commits into from
Jun 6, 2024
Merged

Make GenericParameters return optional instead of empty defaults on non-existant keys #580

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4725084
Add get method returning an optional value
tmadlener Apr 9, 2024
cd5017d
Fix unittests to use non-deprecated methods
tmadlener Apr 9, 2024
e91faa0
Remove test of deprecated type helper
tmadlener Apr 9, 2024
c19d01c
Switch to non-deprecated usage in Frame
tmadlener Apr 10, 2024
541bd00
Make the Frame return optional parameters as well
tmadlener Apr 10, 2024
19a3c5a
Remove deprecated functionality from GenericParameters
tmadlener Apr 10, 2024
d08f436
Remove deprecated functionality from GenericParameters
tmadlener Apr 10, 2024
ad75657
Make getMap private as public access is no longer necessary
tmadlener May 16, 2024
170e242
Bump the patch version to 99 to signify development
tmadlener May 16, 2024
eeadbf7
Make map public again, as it is needed for DD4hep
tmadlener May 16, 2024
c0d5907
Fix comments on documentation
tmadlener May 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is the last version before 1.0 😃

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:D I would hope so. However, the main reason for this is that setting this to 99 would in principle allow us to still have patch releases, while still having pre-processor checks only compare >0.99 effectively.

In principle we could consider doing this by default after tagging a release to make it easier to detect development builds.


SET( ${PROJECT_NAME}_VERSION "${${PROJECT_NAME}_VERSION_MAJOR}.${${PROJECT_NAME}_VERSION_MINOR}.${${PROJECT_NAME}_VERSION_PATCH}" )

Expand Down
13 changes: 5 additions & 8 deletions include/podio/Frame.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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 {
Expand Down
62 changes: 17 additions & 45 deletions include/podio/GenericParameters.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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;
Expand All @@ -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>) {
Expand All @@ -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 {
Expand All @@ -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
Expand All @@ -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>();

Expand Down
8 changes: 6 additions & 2 deletions python/podio/frame.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -190,10 +192,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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed? Do the values disappear if the frame is deleted?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They will definitely disappear if the Frame is gone. but in this case I think it's also something related to std::optional and how it combines with cppyy. I added it to fix tests, but let me check whether it's truly necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to be interaction between std::optional and cppyy. Without the deepcopy, I run into the following in the tests:

130: ======================================================================
130: ERROR: test_frame_parameters (test_Frame.FrameReadTest)
130: Check that all expected parameters are available.
130: ----------------------------------------------------------------------
130: Traceback (most recent call last):
130:   File "/home/tmadlener/work/AIDASoft/podio/python/podio/test_Frame.py", line 192, in test_frame_parameters
130:     self.assertEqual(
130:   File "/home/tmadlener/work/.spack/spackages/python/3.10.13/skylake-ubuntu22.04-gcc12.3.0/jsngvl3/lib/python3.10/unittest/case.py", line 845, in assertEqual
130:     assertion_func(first, second, msg=msg)
130:   File "/home/tmadlener/work/.spack/spackages/python/3.10.13/skylake-ubuntu22.04-gcc12.3.0/jsngvl3/lib/python3.10/unittest/case.py", line 1051, in assertListEqual
130:     self.assertSequenceEqual(list1, list2, msg, seq_type=list)
130:   File "/home/tmadlener/work/.spack/spackages/python/3.10.13/skylake-ubuntu22.04-gcc12.3.0/jsngvl3/lib/python3.10/unittest/case.py", line 976, in assertSequenceEqual
130:     if seq1 == seq2:
130: SystemError: Negative size passed to PyUnicode_FromStringAndSize

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the problem is that the par_value (or it's underlying storage) goes out of scope without the deepcopy. Previously the getParameter returned a const& so it was kept alive by the underlying frame, so no deepcopy was necessary.


# This access already raises the KeyError if there is no such parameter
par_type = self._param_key_types[name]
Expand Down
10 changes: 5 additions & 5 deletions tests/read_python_frame.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
8 changes: 4 additions & 4 deletions tests/read_test.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'");
}
Expand All @@ -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");
}
Expand Down
8 changes: 4 additions & 4 deletions tests/unittests/frame.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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]);
Expand Down
Loading