-
Notifications
You must be signed in to change notification settings - Fork 26
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
Make it possible to transparently migrate to TrackerHit interface #174
Conversation
7edf98d
to
3c8a8ab
Compare
3c8a8ab
to
9de9e48
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After the comments looks fine to go in
test/k4FWCoreTest/src/components/k4FWCoreTest_CreateExampleEventData.h
Outdated
Show resolved
Hide resolved
test/k4FWCoreTest/src/components/ExampleFunctionalProducerMultiple.cpp
Outdated
Show resolved
Hide resolved
test/k4FWCoreTest/src/components/ExampleFunctionalConsumerMultiple.cpp
Outdated
Show resolved
Hide resolved
k4FWCore/components/PodioInput.cpp
Outdated
m_readers["edm4hep::TrackerHitCollection"] = [&](std::string_view collName) { | ||
maybeRead<edm4hep::TrackerHitCollection>(collName); | ||
warning() << "Reading a TrackerHitCollection via TrackerHit3DCollection (" << collName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this warning; the only way you can reach here is when TrackerHitCollection is available (otherwise the type will never be edm4hep::TrackerHitCollection
) in which case TrackerHit3DCollection
is an alias to TrackerHitCollection
meaning this warning wouldn't be true, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be possible to come here from old files, I think. In that case the old name will still be in the file metadata and propagate until here. However, we are fine just reading that through the new type. The warning might not be necessary though, as it will probably do the right thing. Additionally, all the other changes that we are currently introducing will break existing files in any case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are old files going to be readable? I'm quite sure they are not. ROOT finds something called "TrackerHitCollection" that it doesn't know anything about and it's impossible for it to reconstruct a "Tracker3DHitCollection". So this is why I think this warning can't be triggered because if the collections are saved as TrackerHitCollection
then they can't be read
Co-authored-by: Juan Miguel Carceller <[email protected]>
Downstream build fails because there are still some tests in fccsw that are failing... |
BEGINRELEASENOTES
edm4hep::TrackerHit
toedm4hep::TrackerHit3D
in order to keep things working after Use interface types for TrackerHit EDM4hep#252TrackerHit3D
and tpyedef theTrackerHit
toTrackerHit3D
if it does not existENDRELEASENOTES
- [x] Use interface types for TrackerHit EDM4hep#252