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 topoclustering work with new module-theta merged readout #51

Merged

Conversation

giovannimarchiori
Copy link
Contributor

@giovannimarchiori giovannimarchiori commented Sep 27, 2023

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

@BrieucF
Copy link
Contributor

BrieucF commented Sep 28, 2023

Hi Giovanni, thanks!

If I understand correctly what you want to discuss, I see three possibilities:

  1. We go for the if's as you did, knowing that if we try a cos(theta) segmentation or yet something else we will have a list of growing if's, probably not nice
  2. We create a new tool under RecFCCeeCalorimeter. The argument in favor of this is that the theta segmentation does not make sense for FCC-hh and will never be used in this context. It also minimize the modifications we apply to the code that was used for FCC-hh studies (maximizing thus the chances that it will still work for FCC-hh).
  3. We find a way, with an additional layer of abstraction, to have the same treatment regardless of the eta/theta segmentation. This is probably the nicest solution (avoiding if and code duplication) but I don't know if it is at all possible.

@kjvbrt
Copy link
Contributor

kjvbrt commented Sep 28, 2023

Hi all,
I would argue for separate tools/algorithms under RecFCCeeCalorimeter for theta segmentation, since it is not usable for FCChh, as already mentioned. But also, in my opinion, the tools/algorithms should be disposable.

@giovannimarchiori
Copy link
Contributor Author

Hi @BrieucF and @kjvbrt ,
I just pushed some new code, to restore the original ReadNoiseFromFileTool and move my corresponding code for the theta-based readout to RecFCCeeCalorimeter.
Do you think the organisation of the code in this PR (abstracting for a moment for the low-level technical implementation) is OK or do I need to refactor the code in a different way?
Also, what about HEP-FCC/FCCDetectors#63, is it OK to add those extra functions to DetCommon/DetUtils?
if yes then I will then focus on completing the missing things (treatment of diagonal neighbours + cleanup of comments and debug statements)
Thanks!

@kjvbrt
Copy link
Contributor

kjvbrt commented Oct 3, 2023

Hi @giovannimarchiori, I like the organisation of the code.

Do I understand correctly that these algorithms will be used in three different workflows?
First, to create map of neighbours. Second to derive noise and the last one to use both map of neighbours and noise in the reconstruction?
Can you provide example steering files for each?

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 FCCee if at all.

@giovannimarchiori
Copy link
Contributor Author

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

  1. python create_capacitance_file_theta.py
  2. python create_noise_file_chargePreAmp_theta.py
  3. fccrun noise_map_theta.py

create neighbours map

  1. fccrun neighbours_theta.py

run the reconstruction

  1. fccrun run_thetamodulemerged.py

@BrieucF
Copy link
Contributor

BrieucF commented Oct 3, 2023

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 FCCee if at all.

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.

@giovannimarchiori
Copy link
Contributor Author

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.

@kjvbrt
Copy link
Contributor

kjvbrt commented Oct 24, 2023

The failure of the CI tests expected

@kjvbrt kjvbrt merged commit bde81c8 into HEP-FCC:main Oct 24, 2023
1 check failed
@giovannimarchiori giovannimarchiori deleted the gmarchio-main-2023-09-26-newclustering branch April 18, 2024 15:35
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