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

Make it possible to transparently migrate to TrackerHit interface #174

Merged
merged 5 commits into from
Feb 22, 2024

Conversation

tmadlener
Copy link
Contributor

@tmadlener tmadlener commented Jan 19, 2024

BEGINRELEASENOTES

  • Fix usages of edm4hep::TrackerHit to edm4hep::TrackerHit3D in order to keep things working after Use interface types for TrackerHit EDM4hep#252
    • Check for the existance of the TrackerHit3D and tpyedef the TrackerHit to TrackerHit3D if it does not exist

ENDRELEASENOTES

@tmadlener tmadlener changed the title Rename TrackerHit to TrackerHit3D for the concrete type Make it possible to transparently migrate to TrackerHit interface Feb 5, 2024
@tmadlener tmadlener force-pushed the trackerhit-interface branch from 7edf98d to 3c8a8ab Compare February 5, 2024 16:02
@tmadlener tmadlener force-pushed the trackerhit-interface branch from 3c8a8ab to 9de9e48 Compare February 22, 2024 13:18
Copy link
Member

@jmcarcell jmcarcell left a 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

m_readers["edm4hep::TrackerHitCollection"] = [&](std::string_view collName) {
maybeRead<edm4hep::TrackerHitCollection>(collName);
warning() << "Reading a TrackerHitCollection via TrackerHit3DCollection (" << collName
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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]>
@jmcarcell
Copy link
Member

Downstream build fails because there are still some tests in fccsw that are failing...

@jmcarcell jmcarcell merged commit e392581 into key4hep:main Feb 22, 2024
7 of 9 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.

2 participants