Don't use radiusOfInnermostHit for EDM4hep tracks #72

jmcarcell · 2024-06-20T16:56:31Z

and compute it from hits when going from EDM4hep to LCIO. See
key4hep/EDM4hep#326. Uses the utility to compare floats introduced in #71.

Currently there is an issue in the comparisons because in some LCIO collections the radius of the inner most hit doesn't match what you get from the list of hist associated with the track, which isn't enforced anywhere that it should be. One option is to loosen the comparison criteria.

BEGINRELEASENOTES

Don't use radiusOfInnermostHit for EDM4hep tracks, compute it from track state at first hit when going from LCIO to EDM4hep.

ENDRELEASENOTES

tmadlener

Let me check again, where the radius of the innermost hit is actually set for LCIO, resp. where in the reconstruction chain for the input files for the LCIO -> EDM4hep converter tests it is done. Maybe there is an easy way to get to the same result.

k4EDM4hep2LcioConv/include/k4EDM4hep2LcioConv/k4EDM4hep2LcioConv.ipp

tmadlener · 2024-06-26T09:24:58Z

Looking a bit through the iLCSoft reconstruction chain, these are the places where the radius is set non-trivially (i.e. not simply copied from another track). In most of the cases it is set from the referencePoint that is obtained from a TrackState, which according to the documentation most likely is the position of the first hit (if the TrackState is filled appropritately). In the following cases the 3D(!) position is used for this.

Then there are two cases where the same information is used, but only the x-y position is used for calculating the radius:

Finally, there is one occurence, where the position of a TrackerHit is used directly, also in x-y only.

https://github.com/iLCSoft/MarlinTrk/blob/5941fde4b5a24461d60d0bc40c0f75356cbcfad9/src/MarlinTrkUtils.cc#L703-L705

If we want to have the same behavior as for the first three cases here, we would have to shift the filling of this until after we have all the track states converted and then simply use the same approach as there and then document how we fill this information during the conversion from EDM4hep to LCIO.

and compute it from hits when going from EDM4hep to LCIO. See key4hep/EDM4hep#326

tmadlener · 2024-07-02T15:31:13Z

I have introduced a utility function that gets the radius from the track states in a track. It defaults to only using the x and y coordinates, but has a toggle to go to 3D. Currently the 2D version is used in the conversion of the tracks. In order to make all the existing tests pass, we compare against both and are happy if one of them passes. I think the latter is OK because the roundtrip tests pass otherwise as well, and for a direct comparison between converted EDM4hep files and the original LCIO file the "correct choice" depends on which track collection you are looking at and what you actually want to do.

From my side this is ready for review and merge

jmcarcell · 2024-07-03T11:56:25Z

tests/src/CompareEDM4hepLCIO.cc

+  double radius3D = EDM4hep2LCIOConv::getRadiusOfStateAtFirstHit(edm4hepElem, true).value_or(-1.0);
+  const double radiusLCIO = lcioElem->getRadiusOfInnermostHit();
+  if (std::abs(radius - radiusLCIO) > radiusLCIO / 1e6 && std::abs(radius3D - radiusLCIO) > radiusLCIO / 1e6) {
+    std::cerr << "radiusOfInnermostHit in Track (LCIO: " << radiusLCIO << "), EDM4hep: 2d: " << radius


Is this std::cerr because it doesn't always hold for the test files?

The main reason is that all the other ASSERT_XXX macros also go to std::cerr.

jmcarcell · 2024-07-03T11:59:42Z

k4EDM4hep2LcioConv/src/k4EDM4hep2LcioConv.cpp

@@ -147,4 +147,14 @@ std::unique_ptr<lcio::LCEventImpl> convertEvent(const podio::Frame& edmEvent, co
  return lcioEvent;
 }

+std::optional<double> getRadiusOfStateAtFirstHit(const edm4hep::Track& track, bool use3D) {


The use3D parameter as it is will never be used other than in the test below? I guess it doesn't hurt but for someone reading the code it could look like you can have the two ways when the converter defaults to one and there isn't a way of setting the other.

jmcarcell · 2024-07-03T12:05:16Z

I made a couple of comments but this is one of the problems that doesn't have an answer after removing the equivalent quantity. So the possible affected use case would be to go from EDM4hep to LCIO and then using the value of the radius in some processor which I guess no one has (but what if we have tracking in EDM4hep at some point and then keep using the rest of the LCIO chain?). Maybe @Zehvogel can comment on which way he would like the radius to be set. Other than that my doubt is whether to keep the bool parameter or just commit to a single option (even if wrong for some collections).

tmadlener

I agree, there is no right choice here or the conversion. In the ILD files that we have for testing it looks like the choice is not even consistent within a single collection. Currently the 3D version is mainly there as an option to make the tests pass. They also at least document to expect that either of the two options will match expectations.

In principle we could expose the boolean flag of how to calculate the radius also to the convertTracks function, but I am not sure how we would actually expose that through the framework in the end, because that would require additional configuration also there, and since it can in principle differ on a track by track basis not even that might solve everything.

For potential issues, I would wait for them to arise, because then we would have a concrete case to work on rather than speculating.

k4EDM4hep2LcioConv/include/k4EDM4hep2LcioConv/k4EDM4hep2LcioConv.ipp

tmadlener · 2024-07-03T12:02:03Z

tests/src/CompareEDM4hepLCIO.cc

+  double radius3D = EDM4hep2LCIOConv::getRadiusOfStateAtFirstHit(edm4hepElem, true).value_or(-1.0);
+  const double radiusLCIO = lcioElem->getRadiusOfInnermostHit();
+  if (std::abs(radius - radiusLCIO) > radiusLCIO / 1e6 && std::abs(radius3D - radiusLCIO) > radiusLCIO / 1e6) {
+    std::cerr << "radiusOfInnermostHit in Track (LCIO: " << radiusLCIO << "), EDM4hep: 2d: " << radius


The main reason is that all the other ASSERT_XXX macros also go to std::cerr.

Zehvogel · 2024-07-03T12:36:33Z

In the ILD files that we have for testing it looks like the choice is not even consistent within a single collection.

Yes it is quite the mess, but this bad? :o

I think the most consistent one should be the 2D as that is the one set in finaliseLCIOTrack
https://github.com/iLCSoft/MarlinTrk/blob/5941fde4b5a24461d60d0bc40c0f75356cbcfad9/src/MarlinTrkUtils.cc#L703-L705
Which I thought would also be called at least for all "end-user" ILD tracks? :)
It seems to be also what Clupatra does. The forward tracking however prefers the 3D one...
Just adding the search link for completeness
https://github.com/search?q=org%3AiLCSoft+setRadiusOfInnermostHit&type=code

TL; DR:

I agree, there is no right choice here or the conversion.

I agree :(

tmadlener · 2024-07-03T13:29:26Z

Alright, I will merge this then to unblock the EDM4hep PR.

gaede · 2024-07-04T07:37:33Z

For whatever it is worth: the use of a '3d' radius in ForwardTracking is a bug that obviously has gone unnoticed for many years.

jmcarcell mentioned this pull request Jun 20, 2024

Remove radiusOfInnermostHit from tracks key4hep/EDM4hep#326

Merged

tmadlener reviewed Jun 21, 2024

View reviewed changes

k4EDM4hep2LcioConv/include/k4EDM4hep2LcioConv/k4EDM4hep2LcioConv.ipp Outdated Show resolved Hide resolved

tmadlener force-pushed the remove-radius-innermosthit branch from 89dc50e to 97d7639 Compare June 28, 2024 11:45

jmcarcell and others added 4 commits July 2, 2024 17:28

Don't use radiusOfInnermostHit for EDM4hep tracks

a391472

and compute it from hits when going from EDM4hep to LCIO. See key4hep/EDM4hep#326

Fix pre-commit

3e4d903

Introdue utility function to determine radius of innermost hit

475ff4f

Make tests pass again

2bdabbe

tmadlener force-pushed the remove-radius-innermosthit branch from 97d7639 to 2bdabbe Compare July 2, 2024 15:28

Add documentation about radius calculation

c983447

jmcarcell commented Jul 3, 2024

View reviewed changes

tmadlener reviewed Jul 3, 2024

View reviewed changes

Remove unnecessary header

f5d6e80

tmadlener approved these changes Jul 3, 2024

View reviewed changes

tmadlener merged commit e102e37 into main Jul 3, 2024
5 of 7 checks passed

tmadlener deleted the remove-radius-innermosthit branch July 3, 2024 13:54

jmcarcell mentioned this pull request Jul 16, 2024

radiusOfInnermostHit is being computed using 3D information iLCSoft/ForwardTracking#16

Open

tmadlener mentioned this pull request Nov 26, 2024

Question about how to obtain radius of innermost hit key4hep/EDM4hep#383

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't use radiusOfInnermostHit for EDM4hep tracks #72

Don't use radiusOfInnermostHit for EDM4hep tracks #72

jmcarcell commented Jun 20, 2024 •

edited by tmadlener

Loading

tmadlener left a comment

tmadlener commented Jun 26, 2024 •

edited

Loading

tmadlener commented Jul 2, 2024

jmcarcell Jul 3, 2024

tmadlener Jul 3, 2024

jmcarcell Jul 3, 2024

jmcarcell commented Jul 3, 2024

tmadlener left a comment

tmadlener Jul 3, 2024

Zehvogel commented Jul 3, 2024

tmadlener commented Jul 3, 2024

gaede commented Jul 4, 2024

Don't use radiusOfInnermostHit for EDM4hep tracks #72

Don't use radiusOfInnermostHit for EDM4hep tracks #72

Conversation

jmcarcell commented Jun 20, 2024 • edited by tmadlener Loading

tmadlener left a comment

Choose a reason for hiding this comment

tmadlener commented Jun 26, 2024 • edited Loading

tmadlener commented Jul 2, 2024

jmcarcell Jul 3, 2024

Choose a reason for hiding this comment

tmadlener Jul 3, 2024

Choose a reason for hiding this comment

jmcarcell Jul 3, 2024

Choose a reason for hiding this comment

jmcarcell commented Jul 3, 2024

tmadlener left a comment

Choose a reason for hiding this comment

tmadlener Jul 3, 2024

Choose a reason for hiding this comment

Zehvogel commented Jul 3, 2024

tmadlener commented Jul 3, 2024

gaede commented Jul 4, 2024

jmcarcell commented Jun 20, 2024 •

edited by tmadlener

Loading

tmadlener commented Jun 26, 2024 •

edited

Loading