-
Notifications
You must be signed in to change notification settings - Fork 18
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 topoclustering work with new module-theta merged readout #51
make topoclustering work with new module-theta merged readout #51
Conversation
Hi Giovanni, thanks! If I understand correctly what you want to discuss, I see three possibilities:
|
Hi all, |
Hi @BrieucF and @kjvbrt , |
Hi @giovannimarchiori, I like the organisation of the code. Do I understand correctly that these algorithms will be used in three different workflows? This is a nitpick, but might be good to solve now. @BrieucF what naming convention for the algorithms would be most compatible with k4Geo? I mean, where to put |
HI @kjvbrt one has to proceed as described here https://github.com/giovannimarchiori/LAr_scripts/tree/main/geometry but using the _theta scripts in this PR (BrieucF/LAr_scripts#13) instead: create noise map
create neighbours map
run the reconstruction
|
Hi @kjvbrt, "if at all" will depend whether or not other calorimeters (non-FCC) use this repository. Since for now the answer is no I don't think we should try to tackle this in this PR. |
Hi @kjvbrt I think this PR and the related ones are now good for review. I have added the treatment of the diagonal neighbours, cleaned up the code, removed debugging statements, updated the detector parameters to get back 1536 modules. main thing missing is barrel ECAL-HCAL matching (to find neighbours across different volumes) but this can be tackled by a separate PR once we will start looking at hadronic clusters. |
RecFCCeeCalorimeter/src/components/CreateFCCeeCaloNoiseLevelMap.cpp
Outdated
Show resolved
Hide resolved
The failure of the CI tests expected |
This PR and related ones for FCCDetectors and LAr_scripts make topoclustering work with the new theta-module readout for the FCC-ee calorimeter.
I would like to initiate the review to discuss the code structure - I will then remove commented code and debug messages later.
One thing that will probably be debated: I modified the ReadNoiseFromFileTool work for both the phi-eta and module-theta readouts work without creating a new class, but simply adding the possibility to pass to the tool a cell positioning tool, and read from there the eta or theta of cells.
If we decide to go this way, I will polish the code and add a property to tell the tool if it should assume eta or theta bins (in the current quick and dirty implementation, that is backward compatible, it is assumed that if segmentation is used then noise is histogrammed vs |eta|, otherwise vs theta). Otherwise I can create a separate class to read histograms vs theta, but with 95% the same code as that of ReadNoiseFromFileTool ..
Tagging @BrieucF @gartrog @dasphy
Related PRs:
HEP-FCC/FCCDetectors#63
BrieucF/LAr_scripts#13