ldmx-sw icon indicating copy to clipboard operation
ldmx-sw copied to clipboard

add clue clustering

Open tomeichlersmith opened this issue 7 months ago • 2 comments

I am copying over the CLUE clustering, additional analyzer and update to the SimCalorimeterHit that was originally developed in Summer 2024 to study ECal clustering at Lund by Ella Viirola. The visualization code based on Phoenix I plan to move into its own repository and the JSON-producers that prepare an event to be visualized I plan to put into EventDisplay.

What are the issues that this addresses?

This resolves #1411

Check List

  • [x] I successfully compiled ldmx-sw with my developments.
  • [x] I read, understood and follow the coding rules.
  • [x] I ran my developments and the following shows that they are successful.

@Lysarina originally did these developments and includes evidence of their success in the original PR #1416

To Do

  • [ ] is overlay of the sim hits breaking now? See https://github.com/LDMX-Software/ldmx-sw/pull/1416#issuecomment-2757530651

tomeichlersmith avatar May 05 '25 17:05 tomeichlersmith

@bryngemark The fact that the PR Validations pass completely leads me to believe that the issues you were observing before are not present (at least within the clean CI environment.) The total energy is appropriately shifted high and is not the discrete distribution you've shown before.

image

Since this distribution is cut off at higher energies, I'm re-running the it_pileup job locally so that I can make sure it still works above the 12GeV line.

tomeichlersmith avatar May 06 '25 16:05 tomeichlersmith

@bryngemark I have confirmed that, locally and running from scratch, the IT overlay is working as expected. As a double check, I just looked at the EcalVeto_overlay.summedDet_ branch like you have and I see a normal distribution.

image

Without being able to replicate your issue, I would like to ask you to provide more detail on the environment and inputs you used to produce the plot in the original issue. What version of ldmx-sw produced the overlay events? What version produced the "main" events? What version ran overlay? Those sort of questions. If the environment is on SLAC's S3DF, I'd be happy to look at it directly as well if you give me the directory (and I have access to read that directory).

tomeichlersmith avatar May 06 '25 19:05 tomeichlersmith

that's great, @tomeichlersmith. i was getting hopeful when the validation pileup test went through.

i ran on ella's dev branch (that the PR was made from) locally on my laptop. i confirmed that it was behaving differently than the then-current trunk on the same machine (which showed reasonable energies with overlay); that was sometime early fall. i don't think we have any reason to believe whatever happened is present here (in fact we have reasons to believe it isn't) so i suggest we merge and then if i reproduce the bug with new trunk, i make an issue.

bryngemark avatar May 09 '25 18:05 bryngemark

@tvami I have not moved all of the variables in the CLUE implementation to lower_case (amongst other violations of our coding rules) because I fear breaking the implementation when I'm doing it manually. I am currently trying to figure out how to use clang-tidy to try to have it done algorithmically, but I also feel like that is outside the scope of this PR. Can we merge this despite its failure to completely follow the coding rules? I can put my clang-tidy research into an issue and maybe we can have a workflow that enforces more of our coding rules like clang-format does.

tomeichlersmith avatar May 12 '25 15:05 tomeichlersmith

Hi! This might already be resolved or might not even be an issue, but as I wrote in my PR, the initial cluster producer used the WorkingCluster class, that took in EcalHits as pointers. However when I started out I was kind of a pointer noob so I created WorkingEcalCluster, which does essentially the same thing (with some additional improvements), but takes in references instead of pointers in the methods. I never had time to fix this (going back to pointers), so I just wanted to point this out again, in case this would cause some issues later down the line.

Thanks again for taking this on @tomeichlersmith !

Lysarina avatar May 12 '25 15:05 Lysarina