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.

Sign up for GitHub

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

Jump to bottom

Fix ObjectID data type conversions in FarDetectorTrackerCluster #1477

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Fix ObjectID data type conversions in FarDetectorTrackerCluster #1477

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b234ec1
Try swapping auto for podio::ObjectID
simonge May 31, 2024
9adcf37
Merge branch 'main' into fix_CellID_FarClustering
simonge May 31, 2024
cefd91e
Use edm4hep objectid rather than podio
simonge May 31, 2024
0bc9a4e
Merge branch 'fix_CellID_FarClustering' of github.com:eic/EICrecon in…
simonge May 31, 2024
a626503
Merge branch 'main' into fix_CellID_FarClustering
veprbl Jun 1, 2024
997ac48
Merge branch 'main' into fix_CellID_FarClustering
veprbl Jun 4, 2024
3cf07f2
Merge remote-tracking branch 'origin' into fix_CellID_FarClustering
simonge Aug 26, 2024
abfe475
Try setting as edm4hep::ObjectID directly
simonge Aug 26, 2024
765de22
Merge remote-tracking branch 'origin/main' into fix_CellID_FarClustering
simonge Aug 26, 2024
3d7fce6
Try static casting
simonge Aug 26, 2024
3139ed5
Cast the right thing
simonge Aug 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/algorithms/fardetectors/FarDetectorTrackerCluster.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ FarDetectorTrackerCluster::ClusterHits(const edm4eic::RawTrackerHitCollection& i

ROOT::VecOps::RVec<unsigned long> clusterList = {maxIndex};
ROOT::VecOps::RVec<float> clusterT;
std::vector<podio::ObjectID> clusterHits;
std::vector<edm4hep::ObjectID> clusterHits;

// Loop over hits, adding neighbouring hits as relevant
while (clusterList.size()) {
Expand All @@ -134,7 +134,8 @@ FarDetectorTrackerCluster::ClusterHits(const edm4eic::RawTrackerHitCollection& i
clusterList.erase(clusterList.begin());

// Adds raw hit to TrackerHit contribution
clusterHits.push_back((inputHits)[index].getObjectID());
edm4hep::ObjectID hitID = static_cast<edm4hep::ObjectID >((inputHits)[index].getObjectID());
clusterHits.push_back(hitID);

// Energy
auto hitE = e[index];
Expand Down
2 changes: 1 addition & 1 deletion src/algorithms/fardetectors/FarDetectorTrackerCluster.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ struct FDTrackerCluster {
double energy{0.0};
double time{0.0};
double timeError{0.0};
std::vector<podio::ObjectID> rawHits;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment as to why we use edm4hep, not podio (which we use everywhere else) with a link to relevant definition? I would hate for someone to change it back to podio in a year from now.

Also, can this be considered a bug in podio or edm4hep or edm4eic? One would expect these all to have consistent ObjectID types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't quite work out how this was wrong here but the edm4hep::ObjectID type has already been removed from the recent edm4hep releases. This means if we update, the old edm4hep::TrackerHit I'm using will break it anyway as it contains a vector of objectIDs rather than using podio relations. I believe our data model has progressed to a state the work around isn't necessary anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's still missing from edm4eic is a Measurement3D, effectively Measurement2D swapping surface for the extra dimension, through the transformation to global coordinates (or not transforming back to 2D).

std::vector<edm4hep::ObjectID> rawHits;
};
namespace eicrecon {

Expand Down
Loading