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.

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

Add dedicated covariance matrix components and use them #287

Merged
merged 19 commits into from
May 1, 2024

Conversation

tmadlener
Copy link
Contributor

@tmadlener tmadlener commented Mar 14, 2024

BEGINRELEASENOTES

  • Introduce edm4hep::CovMatrix[2,3,4,6}f components to represent covariance matrices
    • using std::array<float, N> under the hood
    • Access functionality that works on enums for defining dimensions and some utilities to do the indexing into the underlying storage
  • Introduce some enum classes for defining dimensions, e.g. TrackParams or Cartesian
  • Use these components instead of the raw std::arrays in datatypes and other components and tie the interpretation of the values to the approriate dimension enum class

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
Copy link
Contributor Author

The release based CI workflows fail because AIDASoft/podio#553 is not yet in there.

@tmadlener
Copy link
Contributor Author

dev3 based workflows are broken due to: root-project/root#14964

edm4hep.yaml Outdated Show resolved Hide resolved
edm4hep.yaml Outdated Show resolved Hide resolved
@tmadlener
Copy link
Contributor Author

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 tmadlener force-pushed the cov-matrix-components branch 2 times, most recently from bf181d6 to 81a41cc Compare April 19, 2024 08:35
@tmadlener
Copy link
Contributor Author

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.

edm4hep.yaml Outdated Show resolved Hide resolved
/// Get the covariance matrix value for the two passed parameters\n
constexpr float getCovMatrix(edm4hep::TrackParams parI, edm4hep::TrackParams parJ) const { return covMatrix.getValue(parI, parJ); }\n
/// Set the covariance matrix value for the two passed parameters\n
constexpr void setCovMatrix(float value, edm4hep::TrackParams parI, edm4hep::TrackParams parJ) { covMatrix.setValue(value, parI, parJ); }
Copy link
Collaborator

Choose a reason for hiding this comment

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

setCovMatrix(std::array const&) missing

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the setCovMatrix be put in MutableExtraCode instead of ExtraCode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely. Let me check again to have that consistently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After having had another look; The TrackState is actually a component. Hence, it only has ExtraCode and it doesn't need a setCovMatrix because you can just assign to covMatrix directly (without having to go through the setter).

@tmadlener
Copy link
Contributor Author

tmadlener commented Apr 30, 2024

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

@hegner hegner merged commit f46cf67 into key4hep:main May 1, 2024
8 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants