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

Removing m_allCells from CaloTopoClusterFCCee algorithm members #70

Merged
merged 3 commits into from
Apr 4, 2024

Conversation

kjvbrt
Copy link
Contributor

@kjvbrt kjvbrt commented Mar 21, 2024

The m_allCells unordered map is to my mind unnecessary as it does not provide any boost in performance, and introduces compulsory zeroing of the map before every event.
Little benchmark on 100 evt:

Runtime:
before [s]: 8.456, 8.603, 8.489, 8.452, 8.661
after [s]: 8.492, 8.578, 8.548, 8.398, 8.568
average before[s]: 8.5322
average after[s]: 8.5168

Also, fixing issue/crash when running over ddsim generated sample(s).

@kjvbrt kjvbrt requested a review from BrieucF March 21, 2024 14:36
@BrieucF
Copy link
Contributor

BrieucF commented Mar 21, 2024

Thanks Juraj, tagging also @giovannimarchiori and @gartrog

@giovannimarchiori
Copy link
Contributor

Hi @kjvbrt ,
since you're touching this code, could you please have a look at why the positions of topoclustered cells in the output root file are in cm while those of other hits/cells/cells after SW clustering are in mm?
I suspect it's this line in CaloTopoClusterFCCee.cpp
newCell.setPosition(edm4hep::Vector3f(posCell.X(), posCell.Y(), posCell.Z()));
where one should divide the 3 coordinates by edm4hep::mm (which is = 0.1)?

Thanks a lot!
Giovanni

@kjvbrt
Copy link
Contributor Author

kjvbrt commented Mar 21, 2024

Hi @giovannimarchiori,
good point, the cell position comes from DD4hep function, which uses cm as base unit. Using dd4hep::mm ...

What about the position of the cluster itself? Is it OK to have it in cm or should I change that as well?

Best,
Juraj

@giovannimarchiori
Copy link
Contributor

right, also the cluster position is in different units for topoclusters (cm) and sw clusters (mm) so would probably a good idea to harmonise and divide cluster position by /mm here

if (er!=1)
info() << "Problem in erasing cell ID from map." << endmsg;
}
cluster.setEnergy(energy);
cluster.setPosition(edm4hep::Vector3f(posX / energy, posY / energy, posZ / energy));
cluster.setPosition(edm4hep::Vector3f{posX / energy, posY / energy, posZ / energy});
Copy link
Contributor

Choose a reason for hiding this comment

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

please divide all 3 coordinates by dd4hep::mm

@giovannimarchiori
Copy link
Contributor

Hello! May I ask what's the status of the review of this PR?

Thanks, Giovanni

@BrieucF
Copy link
Contributor

BrieucF commented Apr 4, 2024

After homogenization of the units it looks good to me!

@kjvbrt kjvbrt merged commit 0d670f1 into HEP-FCC:main Apr 4, 2024
6 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.

3 participants