search

eic / EICrecon

EIC Reconstruction - JANA based
https://eic.github.io/EICrecon
GNU Lesser General Public License v3.0
5 stars 24 forks source link

added functionality to use ZDC Ecal clusters in neutron recon #1512

Closed sebouh137 closed 1 week ago

sebouh137 commented 2 weeks ago

Briefly, what does this PR introduce?

Includes clusters in the ZDC Ecal in the total energy when reconstructing neutrons

What kind of change does this PR introduce?

Please check if this PR fulfills the following:

Does this PR introduce breaking changes? What changes might users need to make to their code?

no

Does this PR change default behavior?

Yes. includes ZDC Ecal clusters in neutron recon.

github-actions[bot] commented 2 weeks ago

Capybara summary for PR 1512

sebouh137 commented 1 week ago

Hi Sebouh, thanks for the development! I alluded to this in one of my comments, but one thing I think this could use is to add a parameter to adjust the relative weighting of the ECal and HCal clusters (even if they're both 1.0 for the time being).

I think that's a good idea to have different corrections for the HCal and ECal. Currently they are set to be exactly the same as one another. One variant of what you propose is we could do is have two sets of parameters, one for the Hcal, and a second set of parameters in the same form for the Ecal. For now, I'll have both parameters be the same, and if this parameterization needs to be tweaked at a later point, someone can do that.

ruse-traveler commented 1 week ago

I think that's a good idea to have different corrections for the HCal and ECal. Currently they are set to be exactly the same as one another. One variant of what you propose is we could do is have two sets of parameters, one for the Hcal, and a second set of parameters in the same form for the Ecal. For now, I'll have both parameters be the same, and if this parameterization needs to be tweaked at a later point, someone can do that.

🙌 Awesome! Sounds good!

sebouh137 commented 1 week ago

I think that's a good idea to have different corrections for the HCal and ECal. Currently they are set to be exactly the same as one another. One variant of what you propose is we could do is have two sets of parameters, one for the Hcal, and a second set of parameters in the same form for the Ecal. For now, I'll have both parameters be the same, and if this parameterization needs to be tweaked at a later point, someone can do that.

🙌 Awesome! Sounds good!

Added this feature. it should be ready now. Let me know if you have anything else you'd like to add, otherwise please approve the pull request. Thanks.

sebouh137 commented 1 week ago

The pre-commit has been acting up today; I'm not sure why it's failing. @ruse-traveler do you have any insights as to why this is happening?

wdconinc commented 1 week ago

githubstatus.com

sebouh137 commented 1 week ago

@ruse-traveler I have added the changes you have requested, and all of the relevant checks have passed. Please approve my pull request, if you don't have any other questions or comments.
Thanks, Sebouh

veprbl commented 1 week ago

I have a question: should this, in principle, do some sort of geometric matching between the two types of clusters? Is that the plan for follow-up changes?

sebouh137 commented 1 week ago

I have a question: should this, in principle, do some sort of geometric matching between the two types of clusters? Is that the plan for follow-up changes?

This is a fair suggestion to include in a future pull request. I think that would add a cut that only includes Ecal clusters within some distance of the most energetic Hcal cluster. But for now let's just get this pull request merged.

ruse-traveler commented 1 week ago

IIRC I believe you mentioned looking at individual clusters/showers for neutron reconstruction in the ZDC in the LFHCAL meeting at some point. That definitely would need some geometric matching between the ECal and HCal clusters...

But agreed! That's for another time; I'm happy with this PR as it is now!