key4hep / EDM4hep

Generic event data model for HEP collider experiments
https://cern.ch/edm4hep
Apache License 2.0
25 stars 35 forks source link

Add dedicated covariance matrix components and use them #287

Closed tmadlener closed 5 months ago

tmadlener commented 6 months ago

BEGINRELEASENOTES

Usage example:

#include "edm4hep/TrackerHit3D.h"

void foo(const edm4hep::TrackerHit3D& hit) {
  const auto covXY  = hit.getCovMatrix(edm4hep::Cartesian::x, edm4hep::Cartesian:y);
}

ENDRELEASENOTES

I am not yet sure whether I really like the duplication of all the extra code here. Especially for the components it is effectively always the same and this could in principle be easily templated. However, then we would have to mix jinja and c++ templates, so I am not entirely sure if we want that.

One possibility would be to put that part of the extra code into some ipp file and just #include it in the ExtraCode declaration.

tmadlener commented 6 months ago

The release based CI workflows fail because https://github.com/AIDASoft/podio/pull/553 is not yet in there.

tmadlener commented 6 months ago

dev3 based workflows are broken due to: https://github.com/root-project/root/issues/14964

tmadlener commented 6 months ago

I have also removed the storage details from the docstrings of the covariance matrix members, as that should be (almost) completely be abstracted away by the components.

tmadlener commented 5 months ago

Because this came up at the EDM4hep meeting: This still has the same issues for RDataFrame::Describe as the version with bare std::array. Trying to run it on the example file that gets created during the tests, I get:

TClass::Init:0: RuntimeWarning: no dictionary for class edm4hep::CovMatrix3f is available
TStreamerInfo::BuildOld:0: RuntimeWarning: Cannot convert edm4hep::TrackerHitPlaneData::covMatrix from type: edm4hep::CovMatrix3f to type: array<float,6>, skip element
---------------------------------------------------------------------------
runtime_error                             Traceback (most recent call last)
Cell In[3], line 1
----> 1 df.Describe()

runtime_error: ROOT::RDF::RDFDescription ROOT::RDF::RInterfaceBase::Describe() =>
    runtime_error: TTree leaf TrackerHitPlanes.covMatrix.values[6] has both a leaf count and a static length. This is not supported.
tmadlener commented 5 months ago

I have removed the setCovMatrix(const std::array<float, N>&) from the datatypes and instead added a constructor of the form

template <typename ...Vs>
CovMatrixNf(Vs... vs) : values{vs...} {}

to the covariance matrix components. This makes direct initialization from initializer lists via the datatypes possible, e.g. as in the tests

  auto trackerHit = MutableTrackerHit3D{};
  trackerHit.setCovMatrix({1.f, 2.f, 3.f, 4.f, 5.f, 6.f});
  REQUIRE(trackerHit.getCovMatrix() == std::array{1.f, 2.f, 3.f, 4.f, 5.f, 6.f});

  const std::array arrValues = {6.f, 5.f, 4.f, 3.f, 2.f, 1.f};
  trackerHit.setCovMatrix(arrValues);
  REQUIRE(trackerHit.getCovMatrix() == std::array{6.f, 5.f, 4.f, 3.f, 2.f, 1.f});

This accepts everything that is convertible to a float and emits a narrowing warning if a narrowing conversion happens (if enabled).