From 8a72bb0c8416892b2a71dddb4f9d0b45949e98a0 Mon Sep 17 00:00:00 2001 From: tmadlener Date: Wed, 12 Jul 2023 18:21:16 +0200 Subject: [PATCH 01/13] Add put method to Frame python bindings --- python/podio/frame.py | 42 ++++++++++++++++++++++++++++++++++---- python/podio/test_Frame.py | 19 +++++++++++++++++ python/podio/test_utils.py | 6 +++++- 3 files changed, 62 insertions(+), 5 deletions(-) diff --git a/python/podio/frame.py b/python/podio/frame.py index 4822b00df..4db8abb7c 100644 --- a/python/podio/frame.py +++ b/python/podio/frame.py @@ -58,6 +58,22 @@ def _get_cpp_vector_types(type_str): return [f'std::vector<{t}>' for t in map(lambda x: x[0], types)] +def _is_collection_base(thing): + """Check whether the passed thing is a podio::CollectionBase + + Args: + thing (any): any object + + Returns: + bool: True if thing is a base of podio::CollectionBase, False otherwise + """ + # Make sure to only instantiate the template with things that cppyy + # understands + if "cppyy" in repr(thing): + return cppyy.gbl.std.is_base_of[cppyy.gbl.podio.CollectionBase, type(thing)].value + return False + + class Frame: """Frame class that serves as a container of collection and meta data.""" @@ -66,6 +82,7 @@ class Frame: # with the correct type that we can compare against _coll_nullptr = cppyy.bind_object(cppyy.nullptr, 'podio::CollectionBase') + def __init__(self, data=None): """Create a Frame. @@ -78,17 +95,16 @@ def __init__(self, data=None): else: self._frame = podio.Frame() - self._collections = tuple(str(s) for s in self._frame.getAvailableCollections()) self._param_key_types = self._init_param_keys() @property def collections(self): - """Get the available collection (names) from this Frame. + """Get the currently available collection (names) from this Frame. Returns: tuple(str): The names of the available collections from this Frame. """ - return self._collections + return tuple(str(s) for s in self._frame.getAvailableCollections()) def get(self, name): """Get a collection from the Frame by name. @@ -107,9 +123,27 @@ def get(self, name): raise KeyError(f"Collection '{name}' is not available") return collection + def put(self, collection, name): + """Put the collection into the frame + + Args: + collection (podio.CollectionBase): The collection to put into the Frame + name (str): The name of the collection + + Returns: + podio.CollectionBase: The reference to the collection that has been put + into the Frame + + Raises: + ValueError: If collection is not actually a podio.CollectionBase + """ + if not _is_collection_base(collection): + raise ValueError("Can only put podio collections into a Frame") + return self._frame.put(cppyy.gbl.std.move(collection), name) + @property def parameters(self): - """Get the available parameter names from this Frame. + """Get the currently available parameter names from this Frame. Returns: tuple (str): The names of the available parameters from this Frame. diff --git a/python/podio/test_Frame.py b/python/podio/test_Frame.py index f8ec1ad96..ee18d95b0 100644 --- a/python/podio/test_Frame.py +++ b/python/podio/test_Frame.py @@ -7,6 +7,8 @@ # using root_io as that should always be present regardless of which backends are built from podio.root_io import Reader +from test_utils import ExampleClusterCollection, ExampleHitCollection + # The expected collections in each frame EXPECTED_COLL_NAMES = { 'arrays', 'WithVectorMember', 'info', 'fixedWidthInts', 'mcparticles', @@ -34,6 +36,23 @@ def test_frame_invalid_access(self): with self.assertRaises(KeyError): _ = frame.get_parameter('NonExistantParameter') + with self.assertRaises(ValueError): + _ = frame.put(list(), "invalid_collection_type") + + def test_frame_put_collection(self): + frame = Frame() + self.assertEqual(frame.collections, tuple()) + + hits = ExampleHitCollection() + hits.create() + hits2 = frame.put(hits, "hits_from_python") + self.assertEqual(frame.collections, tuple(["hits_from_python"])) + # The original collection is gone at this point, and ideally just leaves an + # empty shell + self.assertEqual(len(hits), 0) + # On the other hand the return value of put has the original content + self.assertEqual(len(hits2), 1) + class FrameReadTest(unittest.TestCase): """Unit tests for the Frame python bindings for Frames read from file. diff --git a/python/podio/test_utils.py b/python/podio/test_utils.py index 2c5e282b6..65a264e28 100644 --- a/python/podio/test_utils.py +++ b/python/podio/test_utils.py @@ -3,4 +3,8 @@ import os -SKIP_SIO_TESTS = os.environ.get('SKIP_SIO_TESTS', '1') == '1' +SKIP_SIO_TESTS = os.environ.get("SKIP_SIO_TESTS", "1") == "1" + +import ROOT +ROOT.gSystem.Load("libTestDataModelDict.so") +from ROOT import ExampleHitCollection, ExampleClusterCollection From 54d73bc1beaf4f88ab4d15f475808453d8cad5cb Mon Sep 17 00:00:00 2001 From: tmadlener Date: Wed, 12 Jul 2023 19:06:31 +0200 Subject: [PATCH 02/13] Add a test that writes a file via the python bindings and ROOT --- python/podio/base_writer.py | 24 +++++++++++++++++++ python/podio/root_io.py | 14 +++++++++-- python/podio/test_utils.py | 40 +++++++++++++++++++++++++++++++ tests/root_io/CMakeLists.txt | 5 ++++ tests/root_io/write_frame_root.py | 6 +++++ 5 files changed, 87 insertions(+), 2 deletions(-) create mode 100644 python/podio/base_writer.py create mode 100644 tests/root_io/write_frame_root.py diff --git a/python/podio/base_writer.py b/python/podio/base_writer.py new file mode 100644 index 000000000..268086b38 --- /dev/null +++ b/python/podio/base_writer.py @@ -0,0 +1,24 @@ +#!/usr/bin/env python3 +"""Python module for defining the basic writer interface that is used by the +backend specific bindings""" + + +class BaseWriterMixin: + """Mixin class that defines the base interface of the writers. + + The backend specific writers inherit from here and have to initialize the + following members: + - _writer: The actual writer that is able to write frames + """ + + def write_frame(self, frame, category, collections=None): + """Write the given frame under the passed category, optionally limiting the + collections that are written. + + Args: + frame (podio.frame.Frame): The Frame to write + category (str): The category name + collections (optional, default=None): The subset of collections to + write. If None, all collections are written + """ + self._writer.writeFrame(frame._frame, category, collections or list()) diff --git a/python/podio/root_io.py b/python/podio/root_io.py index a5f25950e..9623ee24d 100644 --- a/python/podio/root_io.py +++ b/python/podio/root_io.py @@ -6,8 +6,7 @@ from ROOT import podio # noqa: E402 # pylint: disable=wrong-import-position from podio.base_reader import BaseReaderMixin # pylint: disable=wrong-import-position - -Writer = podio.ROOTFrameWriter +from podio.base_writer import BaseWriterMixin # pylint: disable=wrong-import-position class Reader(BaseReaderMixin): @@ -49,3 +48,14 @@ def __init__(self, filenames): self._is_legacy = True super().__init__() + + +class Writer(BaseWriterMixin): + """Writer class for writing podio root files""" + def __init__(self, filename): + """Create a writer for writing files + + Args: + filename (str): The name of the output file + """ + self._writer = podio.ROOTFrameWriter(filename) diff --git a/python/podio/test_utils.py b/python/podio/test_utils.py index 65a264e28..95cd00ab5 100644 --- a/python/podio/test_utils.py +++ b/python/podio/test_utils.py @@ -8,3 +8,43 @@ import ROOT ROOT.gSystem.Load("libTestDataModelDict.so") from ROOT import ExampleHitCollection, ExampleClusterCollection + +from podio.frame import Frame + + +def create_hit_collection(): + """Create a simple hit collection with two hits for testing""" + hits = ExampleHitCollection() + hits.create(0xBAD, 0.0, 0.0, 23.0) + hits.create(0xCAFFEE, 1.0, 0.0, 0.0, 12.0) + + return hits + + +def create_cluster_collection(): + """Create a simple cluster collection with two clusters""" + clusters = ExampleClusterCollection() + clu0 = clusters.create() + clu0.energy(3.14) + clu1 = clusters.create() + clu1.energy(1.23) + + return clusters + + +def create_frame(): + """Create a frame with an ExampleHit and an ExampleCluster collection""" + frame = Frame() + frame.put(create_hit_collection(), "hits_from_python") + frame.put(create_cluster_collection(), "cluster_from_python") + + return frame + + +def write_file(WriterT, filename): + """Write a file using the given Writer type and put one Frame into it under + the events category + """ + writer = WriterT(filename) + event = create_frame() + writer.write_frame(event, "events") diff --git a/tests/root_io/CMakeLists.txt b/tests/root_io/CMakeLists.txt index bfa8309f3..3387d2517 100644 --- a/tests/root_io/CMakeLists.txt +++ b/tests/root_io/CMakeLists.txt @@ -69,3 +69,8 @@ if (DEFINED CACHE{PODIO_TEST_INPUT_DATA_DIR}) ADD_PODIO_LEGACY_TEST(${version} read_frame_legacy_root example.root legacy_test_cases) endforeach() endif() + +#--- Write via python and the ROOT backend and see if we can read it back in in +#--- c++ +add_test(NAME write_frame_root_python COMMAND python3 ${CMAKE_CURRENT_LIST_DIR}/write_frame_root.py) +PODIO_SET_TEST_ENV(write_frame_root_python) diff --git a/tests/root_io/write_frame_root.py b/tests/root_io/write_frame_root.py new file mode 100644 index 000000000..40c712ae7 --- /dev/null +++ b/tests/root_io/write_frame_root.py @@ -0,0 +1,6 @@ +#!/usr/bin/env python3 + +from podio import test_utils +from podio.root_io import Writer + +test_utils.write_file(Writer, "example_frame_with_py.root") From d4c1b34ceba81043089651b2aa326f98e7fb80e9 Mon Sep 17 00:00:00 2001 From: tmadlener Date: Wed, 12 Jul 2023 19:34:24 +0200 Subject: [PATCH 03/13] Add a c++ test that reads content written in python --- python/podio/base_writer.py | 2 +- python/podio/test_utils.py | 8 ++-- tests/read_python_frame.h | 60 ++++++++++++++++++++++++ tests/root_io/CMakeLists.txt | 6 ++- tests/root_io/read_python_frame_root.cpp | 7 +++ 5 files changed, 77 insertions(+), 6 deletions(-) create mode 100644 tests/read_python_frame.h create mode 100644 tests/root_io/read_python_frame_root.cpp diff --git a/python/podio/base_writer.py b/python/podio/base_writer.py index 268086b38..881c9fa52 100644 --- a/python/podio/base_writer.py +++ b/python/podio/base_writer.py @@ -21,4 +21,4 @@ def write_frame(self, frame, category, collections=None): collections (optional, default=None): The subset of collections to write. If None, all collections are written """ - self._writer.writeFrame(frame._frame, category, collections or list()) + self._writer.writeFrame(frame._frame, category, collections or frame.collections) diff --git a/python/podio/test_utils.py b/python/podio/test_utils.py index 95cd00ab5..3680f7a78 100644 --- a/python/podio/test_utils.py +++ b/python/podio/test_utils.py @@ -15,7 +15,7 @@ def create_hit_collection(): """Create a simple hit collection with two hits for testing""" hits = ExampleHitCollection() - hits.create(0xBAD, 0.0, 0.0, 23.0) + hits.create(0xBAD, 0.0, 0.0, 0.0, 23.0) hits.create(0xCAFFEE, 1.0, 0.0, 0.0, 12.0) return hits @@ -35,8 +35,10 @@ def create_cluster_collection(): def create_frame(): """Create a frame with an ExampleHit and an ExampleCluster collection""" frame = Frame() - frame.put(create_hit_collection(), "hits_from_python") - frame.put(create_cluster_collection(), "cluster_from_python") + hits = create_hit_collection() + frame.put(hits, "hits_from_python") + clusters = create_cluster_collection() + frame.put(clusters, "clusters_from_python") return frame diff --git a/tests/read_python_frame.h b/tests/read_python_frame.h new file mode 100644 index 000000000..003d95812 --- /dev/null +++ b/tests/read_python_frame.h @@ -0,0 +1,60 @@ +#ifndef PODIO_TESTS_READ_PYTHON_FRAME_H // NOLINT(llvm-header-guard): folder structure not suitable +#define PODIO_TESTS_READ_PYTHON_FRAME_H // NOLINT(llvm-header-guard): folder structure not suitable + +#include "datamodel/ExampleClusterCollection.h" +#include "datamodel/ExampleHitCollection.h" + +#include "podio/Frame.h" + +#include + +int checkHits(const ExampleHitCollection& hits) { + if (hits.size() != 2) { + std::cerr << "There should be two hits in the collection (actual size: " << hits.size() << ")" << std::endl; + return 1; + } + + auto hit1 = hits[0]; + if (hit1.cellID() != 0xbad || hit1.x() != 0.0 || hit1.y() != 0.0 || hit1.z() != 0.0 || hit1.energy() != 23.0) { + std::cerr << "Could not retrieve the correct hit[0]: (expected: " << ExampleHit(0xbad, 0.0, 0.0, 0.0, 23.0) + << ", actual: " << hit1 << ")" << std::endl; + return 1; + } + + auto hit2 = hits[1]; + if (hit2.cellID() != 0xcaffee || hit2.x() != 1.0 || hit2.y() != 0.0 || hit2.z() != 0.0 || hit2.energy() != 12.0) { + std::cerr << "Could not retrieve the correct hit[1]: (expected: " << ExampleHit(0xcaffee, 1.0, 0.0, 0.0, 12.0) + << ", actual: " << hit1 << ")" << std::endl; + return 1; + } + + return 0; +} + +int checkClusters(const ExampleClusterCollection& clusters) { + if (clusters.size() != 2) { + std::cerr << "There should be two clusters in the collection (actual size: " << clusters.size() << ")" << std::endl; + return 1; + } + + if (clusters[0].energy() != 3.14 || clusters[1].energy() != 1.23) { + std::cerr << "Energies of the clusters is wrong: (expected: 3.14 and 1.23, actual " << clusters[0].energy() + << " and " << clusters[1].energy() << ")" << std::endl; + return 1; + } + + return 0; +} + +template +int read_frame(const std::string& filename) { + auto reader = ReaderT(); + reader.openFile(filename); + + auto event = podio::Frame(reader.readEntry("events", 0)); + + return checkHits(event.get("hits_from_python")) + + checkClusters(event.get("clusters_from_python")); +} + +#endif // PODIO_TESTS_READ_PYTHON_FRAME_H diff --git a/tests/root_io/CMakeLists.txt b/tests/root_io/CMakeLists.txt index 3387d2517..5c867a9fe 100644 --- a/tests/root_io/CMakeLists.txt +++ b/tests/root_io/CMakeLists.txt @@ -11,6 +11,7 @@ set(root_dependent_tests write_frame_root.cpp read_frame_legacy_root.cpp read_frame_root_multiple.cpp + read_python_frame_root.cpp ) if(ENABLE_RNTUPLE) set(root_dependent_tests @@ -72,5 +73,6 @@ endif() #--- Write via python and the ROOT backend and see if we can read it back in in #--- c++ -add_test(NAME write_frame_root_python COMMAND python3 ${CMAKE_CURRENT_LIST_DIR}/write_frame_root.py) -PODIO_SET_TEST_ENV(write_frame_root_python) +add_test(NAME write_python_frame_root COMMAND python3 ${CMAKE_CURRENT_LIST_DIR}/write_frame_root.py) +PODIO_SET_TEST_ENV(write_python_frame_root) +set_property(TEST read_python_frame_root PROPERTY DEPENDS write_python_frame_root) diff --git a/tests/root_io/read_python_frame_root.cpp b/tests/root_io/read_python_frame_root.cpp new file mode 100644 index 000000000..23d1c0015 --- /dev/null +++ b/tests/root_io/read_python_frame_root.cpp @@ -0,0 +1,7 @@ +#include "read_python_frame.h" + +#include "podio/ROOTFrameReader.h" + +int main() { + return read_frame("example_frame_with_py.root"); +} From e9bc0da2e17b22258a1f0afd756c6a7e46707d92 Mon Sep 17 00:00:00 2001 From: tmadlener Date: Wed, 12 Jul 2023 19:40:29 +0200 Subject: [PATCH 04/13] Add python bindings for SIO writer and corresponding tests --- python/podio/sio_io.py | 14 ++++++++++++-- tests/sio_io/read_python_frame_sio.cpp | 7 +++++++ tests/sio_io/write_frame_sio.py | 6 ++++++ 3 files changed, 25 insertions(+), 2 deletions(-) create mode 100644 tests/sio_io/read_python_frame_sio.cpp create mode 100644 tests/sio_io/write_frame_sio.py diff --git a/python/podio/sio_io.py b/python/podio/sio_io.py index 01f9d577f..30257a860 100644 --- a/python/podio/sio_io.py +++ b/python/podio/sio_io.py @@ -9,8 +9,7 @@ from ROOT import podio # noqa: 402 # pylint: disable=wrong-import-position from podio.base_reader import BaseReaderMixin # pylint: disable=wrong-import-position - -Writer = podio.SIOFrameWriter +from podio.base_writer import BaseWriterMixin # pylint: disable=wrong-import-position class Reader(BaseReaderMixin): @@ -46,3 +45,14 @@ def __init__(self, filename): self._is_legacy = True super().__init__() + + +class Writer(BaseWriterMixin): + """Writer class for writing podio root files""" + def __init__(self, filename): + """Create a writer for writing files + + Args: + filename (str): The name of the output file + """ + self._writer = podio.SIOFrameWriter(filename) diff --git a/tests/sio_io/read_python_frame_sio.cpp b/tests/sio_io/read_python_frame_sio.cpp new file mode 100644 index 000000000..61c3eb481 --- /dev/null +++ b/tests/sio_io/read_python_frame_sio.cpp @@ -0,0 +1,7 @@ +#include "read_python_frame.h" + +#include "podio/SIOFrameReader.h" + +int main() { + return read_frame("example_frame_with_py.sio"); +} diff --git a/tests/sio_io/write_frame_sio.py b/tests/sio_io/write_frame_sio.py new file mode 100644 index 000000000..cfa360aa4 --- /dev/null +++ b/tests/sio_io/write_frame_sio.py @@ -0,0 +1,6 @@ +#!/usr/bin/env python3 + +from podio import test_utils +from podio.sio_io import Writer + +test_utils.write_file(Writer, "example_frame_with_py.sio") From 6e0c20f6e21028d5a44fed5af81a5ba9ad91fad7 Mon Sep 17 00:00:00 2001 From: Thomas Madlener Date: Thu, 13 Jul 2023 09:39:28 +0200 Subject: [PATCH 05/13] Remove unnecessary lines --- python/podio/frame.py | 1 - 1 file changed, 1 deletion(-) diff --git a/python/podio/frame.py b/python/podio/frame.py index 4db8abb7c..eb9a039e8 100644 --- a/python/podio/frame.py +++ b/python/podio/frame.py @@ -82,7 +82,6 @@ class Frame: # with the correct type that we can compare against _coll_nullptr = cppyy.bind_object(cppyy.nullptr, 'podio::CollectionBase') - def __init__(self, data=None): """Create a Frame. From 00055fb848f6ae62e3c1116e3de6b9e71fa91bf8 Mon Sep 17 00:00:00 2001 From: tmadlener Date: Thu, 13 Jul 2023 09:49:27 +0200 Subject: [PATCH 06/13] Add a put_parameter method to the python Frame --- python/podio/frame.py | 45 ++++++++++++++++++++++++++++++++++++-- python/podio/test_Frame.py | 31 ++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 2 deletions(-) diff --git a/python/podio/frame.py b/python/podio/frame.py index eb9a039e8..ea23d4930 100644 --- a/python/podio/frame.py +++ b/python/podio/frame.py @@ -94,7 +94,7 @@ def __init__(self, data=None): else: self._frame = podio.Frame() - self._param_key_types = self._init_param_keys() + self._param_key_types = self._get_param_keys_types() @property def collections(self): @@ -196,6 +196,47 @@ def _get_param_value(par_type, name): return _get_param_value(vec_types[0], name) + def put_parameter(self, key, value): + """Put a parameter into the Frame. + + Puts a parameter into the Frame after doing some (incomplete) type checks. + If a list is passed the parameter type is determined from looking at the + first element of the list only. Additionally, since python doesn't + differentiate between floats and doubles, floats will always be stored as + double. + + Args: + key (str): The name of the parameter + value (int, float, str or list of these): The parameter value + + Raises: + ValueError: If a non-supported parameter type is passed + """ + # For lists we determine the c++ vector type and use that to call the + # correct template overload explicitly + if isinstance(value, (list, tuple)): + vec_types = _get_cpp_vector_types(type(value[0]).__name__) + if len(vec_types) == 0: + raise ValueError(f"Cannot put a parameter of type {type(value[0])} into a Frame") + + par_type = vec_types[0] + if isinstance(value[0], float): + # Always store floats as doubles from the python side + par_type = par_type.replace("float", "double") + + self._frame.putParameter[par_type](key, value) + else: + # If we have a single integer, a std::string overload kicks in with higher + # priority than the template for some reason. So we explicitly select the + # correct template here + if isinstance(value, int): + self._frame.putParameter["int"](key, value) + else: + self._frame.putParameter(key, value) + + self._param_key_types = self._get_param_keys_types() # refresh the cache + + def get_parameters(self): """Get the complete podio::GenericParameters object stored in this Frame. @@ -233,7 +274,7 @@ def get_param_info(self, name): return par_infos - def _init_param_keys(self): + def _get_param_keys_types(self): """Initialize the param keys dict for easier lookup of the available parameters. Returns: diff --git a/python/podio/test_Frame.py b/python/podio/test_Frame.py index ee18d95b0..be735307f 100644 --- a/python/podio/test_Frame.py +++ b/python/podio/test_Frame.py @@ -40,6 +40,7 @@ def test_frame_invalid_access(self): _ = frame.put(list(), "invalid_collection_type") def test_frame_put_collection(self): + """Check that putting a collection works as expected""" frame = Frame() self.assertEqual(frame.collections, tuple()) @@ -53,6 +54,36 @@ def test_frame_put_collection(self): # On the other hand the return value of put has the original content self.assertEqual(len(hits2), 1) + def test_frame_put_parameters(self): + """Check that putting a parameter works as expected""" + frame = Frame() + self.assertEqual(frame.parameters, tuple()) + + frame.put_parameter("a_string_param", "a string") + self.assertEqual(frame.parameters, tuple(["a_string_param"])) + self.assertEqual(frame.get_parameter("a_string_param"), "a string") + + frame.put_parameter("float_param", 3.14) + self.assertEqual(frame.get_parameter("float_param"), 3.14) + + frame.put_parameter("int", 42) + self.assertEqual(frame.get_parameter("int"), 42) + + frame.put_parameter("string_vec", ["a", "b", "cd"]) + str_vec = frame.get_parameter("string_vec") + self.assertEqual(len(str_vec), 3) + self.assertEqual(str_vec, ["a", "b", "cd"]) + + frame.put_parameter("more_ints", [1, 2345]) + int_vec = frame.get_parameter("more_ints") + self.assertEqual(len(int_vec), 2) + self.assertEqual(int_vec, [1, 2345]) + + frame.put_parameter("float_vec", [1.23, 4.56, 7.89]) + vec = frame.get_parameter("float_vec", as_type="double") + self.assertEqual(len(vec), 3) + self.assertEqual(vec, [1.23, 4.56, 7.89]) + class FrameReadTest(unittest.TestCase): """Unit tests for the Frame python bindings for Frames read from file. From 91db172a530646b67bca7133eb39d2ceb3ceb480 Mon Sep 17 00:00:00 2001 From: tmadlener Date: Thu, 13 Jul 2023 11:05:13 +0200 Subject: [PATCH 07/13] Add I/O tests for put_parameter --- python/podio/test_utils.py | 4 ++++ tests/read_python_frame.h | 35 ++++++++++++++++++++++++++++++++++- 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/python/podio/test_utils.py b/python/podio/test_utils.py index 3680f7a78..310e3f964 100644 --- a/python/podio/test_utils.py +++ b/python/podio/test_utils.py @@ -40,6 +40,10 @@ def create_frame(): clusters = create_cluster_collection() frame.put(clusters, "clusters_from_python") + frame.put_parameter("an_int", 42) + frame.put_parameter("some_floats", [1.23, 7.89, 3.14]) + frame.put_parameter("greetings", ["from", "python"]) + return frame diff --git a/tests/read_python_frame.h b/tests/read_python_frame.h index 003d95812..791905c18 100644 --- a/tests/read_python_frame.h +++ b/tests/read_python_frame.h @@ -46,6 +46,39 @@ int checkClusters(const ExampleClusterCollection& clusters) { return 0; } +template +std::ostream& operator<<(std::ostream& o, const std::vector& vec) { + auto delim = "["; + for (const auto& v : vec) { + o << std::exchange(delim, ", ") << v; + } + return o << "]"; +} + +int checkParameters(const podio::Frame& frame) { + const auto iVal = frame.getParameter("an_int"); + 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>("some_floats"); + 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>("greetings"); + 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; + } + + return 0; +} + template int read_frame(const std::string& filename) { auto reader = ReaderT(); @@ -54,7 +87,7 @@ int read_frame(const std::string& filename) { auto event = podio::Frame(reader.readEntry("events", 0)); return checkHits(event.get("hits_from_python")) + - checkClusters(event.get("clusters_from_python")); + checkClusters(event.get("clusters_from_python")) + checkParameters(event); } #endif // PODIO_TESTS_READ_PYTHON_FRAME_H From eb0e573f8a6d15bcc818edbfc529d24c8fd9107b Mon Sep 17 00:00:00 2001 From: tmadlener Date: Thu, 13 Jul 2023 11:20:43 +0200 Subject: [PATCH 08/13] [format] Fix flake8 complaints --- python/podio/frame.py | 1 - python/podio/root_io.py | 1 - python/podio/test_Frame.py | 2 +- 3 files changed, 1 insertion(+), 3 deletions(-) diff --git a/python/podio/frame.py b/python/podio/frame.py index ea23d4930..c0c517dfa 100644 --- a/python/podio/frame.py +++ b/python/podio/frame.py @@ -236,7 +236,6 @@ def put_parameter(self, key, value): self._param_key_types = self._get_param_keys_types() # refresh the cache - def get_parameters(self): """Get the complete podio::GenericParameters object stored in this Frame. diff --git a/python/podio/root_io.py b/python/podio/root_io.py index 9623ee24d..a3ab81f40 100644 --- a/python/podio/root_io.py +++ b/python/podio/root_io.py @@ -8,7 +8,6 @@ from podio.base_reader import BaseReaderMixin # pylint: disable=wrong-import-position from podio.base_writer import BaseWriterMixin # pylint: disable=wrong-import-position - class Reader(BaseReaderMixin): """Reader class for reading podio root files.""" diff --git a/python/podio/test_Frame.py b/python/podio/test_Frame.py index be735307f..ecf384dcd 100644 --- a/python/podio/test_Frame.py +++ b/python/podio/test_Frame.py @@ -7,7 +7,7 @@ # using root_io as that should always be present regardless of which backends are built from podio.root_io import Reader -from test_utils import ExampleClusterCollection, ExampleHitCollection +from test_utils import ExampleHitCollection # The expected collections in each frame EXPECTED_COLL_NAMES = { From b5f3eee5b331f269c36ba46b7cab377f04621904 Mon Sep 17 00:00:00 2001 From: tmadlener Date: Thu, 13 Jul 2023 11:24:28 +0200 Subject: [PATCH 09/13] Fix pylint complaints --- python/podio/root_io.py | 1 + python/podio/test_Frame.py | 2 +- python/podio/test_utils.py | 14 +++++++------- tests/root_io/write_frame_root.py | 1 + tests/sio_io/write_frame_sio.py | 1 + 5 files changed, 11 insertions(+), 8 deletions(-) diff --git a/python/podio/root_io.py b/python/podio/root_io.py index a3ab81f40..9623ee24d 100644 --- a/python/podio/root_io.py +++ b/python/podio/root_io.py @@ -8,6 +8,7 @@ from podio.base_reader import BaseReaderMixin # pylint: disable=wrong-import-position from podio.base_writer import BaseWriterMixin # pylint: disable=wrong-import-position + class Reader(BaseReaderMixin): """Reader class for reading podio root files.""" diff --git a/python/podio/test_Frame.py b/python/podio/test_Frame.py index ecf384dcd..f69c074e9 100644 --- a/python/podio/test_Frame.py +++ b/python/podio/test_Frame.py @@ -7,7 +7,7 @@ # using root_io as that should always be present regardless of which backends are built from podio.root_io import Reader -from test_utils import ExampleHitCollection +from podio.test_utils import ExampleHitCollection # The expected collections in each frame EXPECTED_COLL_NAMES = { diff --git a/python/podio/test_utils.py b/python/podio/test_utils.py index 310e3f964..9a8d587ed 100644 --- a/python/podio/test_utils.py +++ b/python/podio/test_utils.py @@ -2,14 +2,14 @@ """Utilities for python unittests""" import os +import ROOT +ROOT.gSystem.Load("libTestDataModelDict.so") # noqa: E402 +from ROOT import ExampleHitCollection, ExampleClusterCollection # noqa: E402 # pylint: disable=wrong-import-position -SKIP_SIO_TESTS = os.environ.get("SKIP_SIO_TESTS", "1") == "1" +from podio.frame import Frame # pylint: disable=wrong-import-position -import ROOT -ROOT.gSystem.Load("libTestDataModelDict.so") -from ROOT import ExampleHitCollection, ExampleClusterCollection -from podio.frame import Frame +SKIP_SIO_TESTS = os.environ.get("SKIP_SIO_TESTS", "1") == "1" def create_hit_collection(): @@ -47,10 +47,10 @@ def create_frame(): return frame -def write_file(WriterT, filename): +def write_file(writer_type, filename): """Write a file using the given Writer type and put one Frame into it under the events category """ - writer = WriterT(filename) + writer = writer_type(filename) event = create_frame() writer.write_frame(event, "events") diff --git a/tests/root_io/write_frame_root.py b/tests/root_io/write_frame_root.py index 40c712ae7..38bece171 100644 --- a/tests/root_io/write_frame_root.py +++ b/tests/root_io/write_frame_root.py @@ -1,4 +1,5 @@ #!/usr/bin/env python3 +"""Script to write a Frame in ROOT format""" from podio import test_utils from podio.root_io import Writer diff --git a/tests/sio_io/write_frame_sio.py b/tests/sio_io/write_frame_sio.py index cfa360aa4..94e08aa27 100644 --- a/tests/sio_io/write_frame_sio.py +++ b/tests/sio_io/write_frame_sio.py @@ -1,4 +1,5 @@ #!/usr/bin/env python3 +"""Script to write a Frame in SIO format""" from podio import test_utils from podio.sio_io import Writer From 6410bfc3798a8ea12daf075f02b9ca3ae33ef5fd Mon Sep 17 00:00:00 2001 From: tmadlener Date: Fri, 14 Jul 2023 10:34:20 +0200 Subject: [PATCH 10/13] Ignore new tests in sanitizer builds --- tests/CTestCustom.cmake | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/CTestCustom.cmake b/tests/CTestCustom.cmake index b0e683f65..d4d05cd2a 100644 --- a/tests/CTestCustom.cmake +++ b/tests/CTestCustom.cmake @@ -22,6 +22,8 @@ if ((NOT "@FORCE_RUN_ALL_TESTS@" STREQUAL "ON") AND (NOT "@USE_SANITIZER@" STREQ read-legacy-files-root_v00-13 read_frame_legacy_root read_frame_root_multiple + write_python_frame_root + read_python_frame_root write_frame_root read_frame_root @@ -35,6 +37,8 @@ if ((NOT "@FORCE_RUN_ALL_TESTS@" STREQUAL "ON") AND (NOT "@USE_SANITIZER@" STREQ write_frame_sio read_frame_sio read_frame_legacy_sio + write_python_frame_sio + read_python_frame_sio write_ascii From e2e9026342e8ca482dad1431cedc8d2df7b6d162 Mon Sep 17 00:00:00 2001 From: tmadlener Date: Thu, 20 Jul 2023 15:16:28 +0200 Subject: [PATCH 11/13] Add an as_type argument to put_parameter to make explicit typing possible --- python/podio/frame.py | 34 ++++++++++++++++++++++++++-------- python/podio/test_Frame.py | 8 ++++++++ python/podio/test_utils.py | 2 ++ tests/read_python_frame.h | 13 +++++++++++++ 4 files changed, 49 insertions(+), 8 deletions(-) diff --git a/python/podio/frame.py b/python/podio/frame.py index c0c517dfa..925a6a46b 100644 --- a/python/podio/frame.py +++ b/python/podio/frame.py @@ -48,13 +48,19 @@ def _determine_cpp_type(idx_and_type): SUPPORTED_PARAMETER_TYPES = _determine_supported_parameter_types() -def _get_cpp_vector_types(type_str): - """Get the possible std::vector from the passed py_type string.""" - # Gather a list of all types that match the type_str (c++ or python) +def _get_cpp_types(type_str): + """Get all possible c++ types from the passed py_type string.""" types = list(filter(lambda t: type_str in t, SUPPORTED_PARAMETER_TYPES)) if not types: raise ValueError(f'{type_str} cannot be mapped to a valid parameter type') + return types + + +def _get_cpp_vector_types(type_str): + """Get the possible std::vector from the passed py_type string.""" + # Gather a list of all types that match the type_str (c++ or python) + types = _get_cpp_types(type_str) return [f'std::vector<{t}>' for t in map(lambda x: x[0], types)] @@ -196,18 +202,23 @@ def _get_param_value(par_type, name): return _get_param_value(vec_types[0], name) - def put_parameter(self, key, value): + def put_parameter(self, key, value, as_type=None): """Put a parameter into the Frame. Puts a parameter into the Frame after doing some (incomplete) type checks. If a list is passed the parameter type is determined from looking at the first element of the list only. Additionally, since python doesn't differentiate between floats and doubles, floats will always be stored as - double. + doubles by default, use the as_type argument to change this if necessary. Args: key (str): The name of the parameter value (int, float, str or list of these): The parameter value + as_type (str, optional): Explicitly specify the type that should be used + to put the parameter into the Frame. Python types (e.g. "str") will + be converted to c++ types. This will override any automatic type + deduction that happens otherwise. Note that this will be taken at + pretty much face-value and there are only limited checks for this. Raises: ValueError: If a non-supported parameter type is passed @@ -215,9 +226,10 @@ def put_parameter(self, key, value): # For lists we determine the c++ vector type and use that to call the # correct template overload explicitly if isinstance(value, (list, tuple)): - vec_types = _get_cpp_vector_types(type(value[0]).__name__) + type_name = as_type or type(value[0]).__name__ + vec_types = _get_cpp_vector_types(type_name) if len(vec_types) == 0: - raise ValueError(f"Cannot put a parameter of type {type(value[0])} into a Frame") + raise ValueError(f"Cannot put a parameter of type {type_name} into a Frame") par_type = vec_types[0] if isinstance(value[0], float): @@ -226,10 +238,16 @@ def put_parameter(self, key, value): self._frame.putParameter[par_type](key, value) else: + if as_type is not None: + cpp_types = _get_cpp_types(as_type) + if len(cpp_types) == 0: + raise ValueError(f"Cannot put a parameter of type {as_type} into a Frame") + self._frame.putParameter[cpp_types[0]](key, value) + # If we have a single integer, a std::string overload kicks in with higher # priority than the template for some reason. So we explicitly select the # correct template here - if isinstance(value, int): + elif isinstance(value, int): self._frame.putParameter["int"](key, value) else: self._frame.putParameter(key, value) diff --git a/python/podio/test_Frame.py b/python/podio/test_Frame.py index f69c074e9..3d50073c6 100644 --- a/python/podio/test_Frame.py +++ b/python/podio/test_Frame.py @@ -84,6 +84,14 @@ def test_frame_put_parameters(self): self.assertEqual(len(vec), 3) self.assertEqual(vec, [1.23, 4.56, 7.89]) + frame.put_parameter("real_float_vec", [1.23, 4.56, 7.89], as_type="float") + f_vec = frame.get_parameter("real_float_vec", as_type="float") + self.assertEqual(len(f_vec), 3) + self.assertEqual(vec, [1.23, 4.56, 7.89]) + + frame.put_parameter("float_as_float", 3.14, as_type="float") + self.assertAlmostEqual(frame.get_parameter("float_as_float"), 3.14, places=5) + class FrameReadTest(unittest.TestCase): """Unit tests for the Frame python bindings for Frames read from file. diff --git a/python/podio/test_utils.py b/python/podio/test_utils.py index 9a8d587ed..44efc9cce 100644 --- a/python/podio/test_utils.py +++ b/python/podio/test_utils.py @@ -43,6 +43,8 @@ def create_frame(): frame.put_parameter("an_int", 42) frame.put_parameter("some_floats", [1.23, 7.89, 3.14]) frame.put_parameter("greetings", ["from", "python"]) + frame.put_parameter("real_float", 3.14, as_type="float") + frame.put_parameter("more_real_floats", [1.23, 4.56, 7.89], as_type="float") return frame diff --git a/tests/read_python_frame.h b/tests/read_python_frame.h index 791905c18..5a06cc4ce 100644 --- a/tests/read_python_frame.h +++ b/tests/read_python_frame.h @@ -76,6 +76,19 @@ int checkParameters(const podio::Frame& frame) { return 1; } + const auto realFloat = frame.getParameter("real_float"); + 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>("more_real_floats"); + 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; + } + return 0; } From fef1d004612aebcc0b7bea9228e8b779d2a4ec76 Mon Sep 17 00:00:00 2001 From: tmadlener Date: Thu, 20 Jul 2023 15:48:10 +0200 Subject: [PATCH 12/13] [pylint] Fix / ignore warnings --- python/podio/base_writer.py | 1 + python/podio/test_Frame.py | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/python/podio/base_writer.py b/python/podio/base_writer.py index 881c9fa52..6f2b5777a 100644 --- a/python/podio/base_writer.py +++ b/python/podio/base_writer.py @@ -21,4 +21,5 @@ def write_frame(self, frame, category, collections=None): collections (optional, default=None): The subset of collections to write. If None, all collections are written """ + # pylint: disable-next=protected-access self._writer.writeFrame(frame._frame, category, collections or frame.collections) diff --git a/python/podio/test_Frame.py b/python/podio/test_Frame.py index 3d50073c6..c09143056 100644 --- a/python/podio/test_Frame.py +++ b/python/podio/test_Frame.py @@ -37,7 +37,8 @@ def test_frame_invalid_access(self): _ = frame.get_parameter('NonExistantParameter') with self.assertRaises(ValueError): - _ = frame.put(list(), "invalid_collection_type") + collection = [1, 2, 4] + _ = frame.put(collection, "invalid_collection_type") def test_frame_put_collection(self): """Check that putting a collection works as expected""" From 06ab6dac73f76eea5c65c963b500b6b951d588dc Mon Sep 17 00:00:00 2001 From: tmadlener Date: Thu, 20 Jul 2023 15:58:22 +0200 Subject: [PATCH 13/13] Update documentation --- python/podio/frame.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/python/podio/frame.py b/python/podio/frame.py index 925a6a46b..1a69d7a46 100644 --- a/python/podio/frame.py +++ b/python/podio/frame.py @@ -131,13 +131,18 @@ def get(self, name): def put(self, collection, name): """Put the collection into the frame + The passed collectoin is "moved" into the Frame, i.e. it cannot be used any + longer after a call to this function. This also means that only objects that + were in the collection at the time of calling this function will be + available afterwards. + Args: collection (podio.CollectionBase): The collection to put into the Frame name (str): The name of the collection Returns: podio.CollectionBase: The reference to the collection that has been put - into the Frame + into the Frame. NOTE: That mutating this collection is not allowed. Raises: ValueError: If collection is not actually a podio.CollectionBase