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

Introduce a global object map that is shared by all converter instances #142

Merged
merged 14 commits into from
Oct 3, 2023

Conversation

tmadlener
Copy link
Contributor

@tmadlener tmadlener commented Sep 13, 2023

BEGINRELEASENOTES

  • Introduce a global object map that is shared by all converter instances (in either direction). This is necessary to properly set all relations even if the related objects are not converted in the same converter instance.
  • Add a global_converter_maps test case (and a description) that checks that this works as expected.

ENDRELEASENOTES

This should solve #113 in the end.

@tmadlener
Copy link
Contributor Author

Things should be running now, but some of the tests are currently failing. I am not yet sure whether the tests are checking for now no longer correct behavior, or whether these are still actual failures.

edm_converters

The "test case" that is failing is the roundtrip conversion from EDM4hep -> LCIO -> EDM4hep of SimTrackerHits. The failing part is the comparison of the relations to the MC particles. The test tries to ensure that the MCParticles of the roundtrip converted SimTrackerHits still point to the original MC particles. However, they actually point to the converted MC particles. I am not sure what would be the expected behavior in this case. I am also not sure whether it is easily possible to actively choose either behavior...

Has been moved to k4EDM4hep2LcioConv (where it fits better)
@tmadlener
Copy link
Contributor Author

This now seems to solve (almost) all issues that were previously present in the clicRec_edm4hep_input test case. All log outputs of the form

[...]
Cannot find corresponding EDM4hep object for an LCIO object in a subset collection of type edm4hep::MCParticle
[...]
Cannot find corresponding EDM4hep Tracks for a LCIO Tracks, while trying to resolve the ReconstructedParticle Relations
[...]
Couldn't find CaloHit to add to Relations for Clusters in edm
[...]
Cannot find corresponding EDM4hep object for an LCIO object in a subset collection of type edm4hep::ReconstructedParticle
[...]
Cannot find corresponding EDM4hep object for an LCIO object in a subset collection of type edm4hep::Cluster
[...]

are now gone with the exception of a few vertices that still cannot find their corresponding reco particle. Here we might just be missing the conversion of a collection(?). I will still try to add a "minimal" (as minimal as possible) smaller test case that just tests the global map instead of everything in the reconstructoin.

@tmadlener tmadlener changed the title [WIP] Unified converter maps Introduce a global object map that is shared by all converter instances Sep 28, 2023
@jmcarcell jmcarcell merged commit 2f86fee into key4hep:main Oct 3, 2023
1 of 7 checks passed
@jmcarcell
Copy link
Contributor

jmcarcell commented Oct 3, 2023

Unintended merge that is already reverted. Can you reopen this one and #144? Sorry. I'm afraid that even the changes are not there for github this has been merged forever 😢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants