Allow to specify units

[hegner]

BEGINRELEASENOTES

• Allow to specify units as part of the datamodel definition

ENDRELEASENOTES

This WIP is to allow the explicit inclusion of units into the datamodel definition.

• Extend the regular expressions/parsing to allow a unit description
• Documentation

[hegner]

@tmadlener - can you let me know whether you'd agree with this? I needed some break from genreflex work and did this long-standing item on my wish list.

[tmadlener]

I think this is a good idea. It is not yet clear to me if/how this will affect the generated code. That might have quite an impact on the size of this feature.

Would it mainly be an addition in the docstring, or would it actually enter in the generated c++ code. If the latter, which base units do we use, or do we leave that choice to the users (and if so, how)? Would we try to make this rather strict and introduce a proper units library to do all the conversions for us (there is a propoal for c++29 to have this in the standard library)?

[hegner]

For the moment it is for documentation purposes. And I want to have a detection in the schema evolution in case it changes. I do not think we should use a units library unless being asked for explicitly.

[tmadlener]

OK. That sounds reasonable to achieve then. Then the main comment/question I have is whether we also need this for the components?

[hegner]

Obviously :-)

[tmadlener]

Just for completeness, could we add this to the generated code (docstring), so that it also shows up in some way in the c++ code.

podio/python/podio/generator_utils.py

Lines 153 to 168 in e0cb135

	def __str__(self):
	"""string representation"""
	# Make sure to include scope-operator if necessary
	if self.namespace:
	scoped_type = f'::{self.namespace}::{self.bare_type}'
	else:
	scoped_type = self.full_type
	if self.default_val:
	definition = rf'{scoped_type} {self.name}{{{self.default_val}}};'
	else:
	definition = rf'{scoped_type} {self.name}{{}};'
	if self.description:
	definition += rf' ///< {self.description}'
	return definition

[tmadlener]

Does this already work for components as well? If so, I think this could be merged.

[hegner]

Yes it does. The test case covers NotSoSimpleStruct (component) and ExampleHit (datatype).

[tmadlener]

Ah right, sorry I missed that because it also has a description. Then more specifically does this also work with components that do not have a description, e.g. the SimpleStruct?

[hegner]

Sure. The description is default initialized as empty string (""), thus there is no special case to handle

[tmadlener]

I was wondering more about the yaml parsing side here. IIRC I special cased the components there to not require a description, and I am not sure if this already covers that special case

[hegner]

The combined regex would give a None for that part and we are back to default handling later. That's the nice thing of having unified the syntax of components and datatypes.

[tmadlener]

OK. IIUC, everything should work from the parsing side and nothing breaks if I add units, e.g. here:

podio/tests/datalayout.yaml

Lines 13 to 17 in dbf9425

	Members:
	- int x
	- int y
	- int z
	- std::array<int, 4> p

However, I think in the current implementation these units would also not end up in a docstring, because they are never passed to the MemberVariable in this case, right? (The bare_<xyz> regexes do not have the unit_str yet, and so the unit is also not passed to the MemberVariable constructor in the end).

[hegner]

@tmadlener - I addressed your comment with a fix. Indeed units were not handled correctly there.

[tmadlener]

It looks like the clang12 based workflows are failing due to not having root in their environment. I suspect that these are transient issues in the LCG nightlies and they should be safe to ignore. The key4hep (release) based one is failing due to an unsuitable ROOT version, so failure there is also expected.

[andresailer]

The clang12 nightlies no longer exist

[tmadlener]

@hegner could you rebase this onto the latest master (or merge that in), so that we pick up a completely working CI again. After that finishes successfully, I would merge this.

[hegner]

@tmadlener - seems all green now :-)