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

[tmadlener]

BEGINRELEASENOTES

• Make the GenericParameters return std::optional on non-existant keys instead of empty defaults. This allows to differentiate between empty (but set) and unset parameters, as the default value could actually be a valid parameter value for some types (e.g. integers or floats).
  • Rename the setValue and getValue functions to just set and get. This is also a breaking change, but as far as we are aware the GenericParameters are only used internally in podio.
• Make the Frame::getParameter also return this std::optional. This is a breaking change that might require adaptation of existing code

ENDRELEASENOTES

Fixes #576

The following PRs make this introduction work transparently and need to be merged first to not break the nightlies

• Transparently introduce switch to optional param values key4hep/EDM4hep#305
• Transparently introduce switch to optional parameter values DD4hep#1264
• Make MetaDataHandle::get throw in case the value is not available and add more overloads key4hep/k4FWCore#195
• Transparently switch to optional parameter values key4hep/k4Clue#59
• Transparently switch to optional parameter values key4hep/k4EDM4hep2LcioConv#64

[m-fila]

I think getValue is an unintuitive name for a method returning optional instead of "value".
Code like parameter.GetValue<int>("foo").has_value() reads a bit awkward

[tmadlener]

Since we are breaking interfaces in any case I don't mind different names. That would in principle allow us to properly deprecate getValue as it is. On the other hand GenericParameters is mainly used internally, so we could probably also just use new names without deprecation.

[m-fila]

podio/include/podio/Frame.h

Lines 290 to 298 in 5e95c36

	/// 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.
	///
	/// @returns The internally used GenericParameters
	inline const podio::GenericParameters& getParameters() const {
	return m_self->parameters();
	}

I think the comment here should recommend usage of getParameter instead of getParameters

[hegner]

@tmadlener - ping. What's the status?

[tmadlener]

Currently testing what is breaking downstream (and then fixing that if necessary)

[tmadlener]

I think getValue is an unintuitive name for a method returning optional instead of "value". Code like parameter.GetValue<int>("foo").has_value() reads a bit awkward

I have changed this to get as we are breaking things in any case

[jmcarcell]

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

[tmadlener]

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.

[tmadlener]

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

[tmadlener]

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.

[jmcarcell]

So this is the last version before 1.0 😃

[tmadlener]

: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.

[tmadlener]

@Zehvogel a quick heads up:

• Rename the setValue and getValue functions to just set and get. This is also a breaking change, but as far as we are aware the GenericParameters are only used internally in podio.

This bit here will require some changes to your RDataFrame code where you directly talk to the PARAMETERS branch, mainly

- df = df.Define("xsec_fb", "PARAMETERS.getValue<float>(\"crossSection\")")
+ df = df.Define("xsec_fb", "PARAMETERS.get<float>(\"crossSection\")")

[Zehvogel]

This bit here will require some changes to your RDataFrame code where you directly talk to the PARAMETERS branch, mainly

actually needed a

- df = df.Define("xsec_fb", "PARAMETERS.getValue<float>(\"crossSection\")")
+ df = df.Define("xsec_fb", "*PARAMETERS.get<float>(\"crossSection\")")

(or something intelligent :))

[tmadlener]

Ah yes, because it now also returns a std::optional. so, either operator* (as in your diff) or .value() will get you the actual value and not the optional).