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

ZDC: LYSO + SiPM-on-tile #1221

Merged
merged 134 commits into from
Jan 23, 2024
Merged

ZDC: LYSO + SiPM-on-tile #1221

merged 134 commits into from
Jan 23, 2024

Conversation

sebouh137
Copy link
Contributor

Briefly, what does this PR introduce?

Adds functionality to run HEXPLIT and other algorithms for the reconstruction of clusters in the SiPM-on-tile ZDC. Also increases the dynamic range for the ZDC Ecal (LYSO crystal).

What kind of change does this PR introduce?

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • Changes have been communicated to collaborators

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

no.

Does this PR change default behavior?

Changes the values of the parameters for the ZDC ECal digitization.

bschmookler and others added 25 commits November 14, 2023 14:26
…d HexGridXY as a recognized segmentation in src/algorithms/calorimetry/CalorimeterHitReco.cc
…ory.h; also changed HEXPLIT to use Emin_in_MIPs instead of Emin
dyRangeZDC set to 800 MeV for ZDC
removed duplicated include statement
restore ZDC ECal reconstruction (this will correspond to the LYSO part of the ZDC)
restored ZDC Ecal recon
restored ZDC ECal recon for cluster clusters
restored part of the ZDC Ecal code.
clean up non-functional changes to code.
changed tabs to spaces.
@github-actions github-actions bot added topic: calorimetry relates to calorimetry topic: far-forward Far forward reconstruction labels Jan 12, 2024
@sebouh137 sebouh137 requested a review from nathanwbrei January 12, 2024 22:59
@wdconinc
Copy link
Contributor

@wdconinc
Copy link
Contributor

Don't worry about the Eigen/Householder include (only that one). It seems that IWYU has trouble with it. Sometimes it just spends 6 hours going back and forth until the job times out or someone cancels it: https://github.com/eic/EICrecon/actions/runs/7532222215/job/20503403799?pr=1234.

@sebouh137
Copy link
Contributor Author

Don't worry about the Eigen/Householder include (only that one). It seems that IWYU has trouble with it. Sometimes it just spends 6 hours going back and forth until the job times out or someone cancels it: https://github.com/eic/EICrecon/actions/runs/7532222215/job/20503403799?pr=1234.

@wdconinc , In this case, there is only one other check that my pull request fails. Namely, getting the deployment of the docs. I don't see anything in the logs that is specific to any of the files I have added or changed. Can this pull request be approved without passing the deploy-docs?

image

…tions on neighboring cells that used for defining the subcells on the current cell, and the number of neighbors who overlap on a given subcell. These numbers are 12, 12, and 3
@veprbl
Copy link
Member

veprbl commented Jan 22, 2024

Don't worry about the Eigen/Householder include (only that one). It seems that IWYU has trouble with it. Sometimes it just spends 6 hours going back and forth until the job times out or someone cancels it: https://github.com/eic/EICrecon/actions/runs/7532222215/job/20503403799?pr=1234.

@wdconinc , In this case, there is only one other check that my pull request fails. Namely, getting the deployment of the docs. I don't see anything in the logs that is specific to any of the files I have added or changed. Can this pull request be approved without passing the deploy-docs?

Please ignore that one. I'm working to resolve it in #1245

Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

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

Need to sort out the naming convention.

src/detectors/ZDC/ZDC.cc Outdated Show resolved Hide resolved
src/detectors/ZDC/ZDC.cc Outdated Show resolved Hide resolved
src/detectors/ZDC/ZDC.cc Outdated Show resolved Hide resolved
src/detectors/ZDC/ZDC.cc Outdated Show resolved Hide resolved
src/services/io/podio/JEventProcessorPODIO.cc Outdated Show resolved Hide resolved
src/services/io/podio/JEventProcessorPODIO.cc Show resolved Hide resolved
@sebouh137
Copy link
Contributor Author

Don't worry about the Eigen/Householder include (only that one). It seems that IWYU has trouble with it. Sometimes it just spends 6 hours going back and forth until the job times out or someone cancels it: https://github.com/eic/EICrecon/actions/runs/7532222215/job/20503403799?pr=1234.

@wdconinc , In this case, there is only one other check that my pull request fails. Namely, getting the deployment of the docs. I don't see anything in the logs that is specific to any of the files I have added or changed. Can this pull request be approved without passing the deploy-docs?

Please ignore that one. I'm working to resolve it in #1245

In that case, all of the conversations have been resolved, and all of the other checks have been fulfilled. @wdconinc @veprbl or @nathanwbrei , could you please approve of this pull request?
Thanks,
Sebouh

Copy link
Contributor

@nathanwbrei nathanwbrei left a comment

Choose a reason for hiding this comment

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

Looks good! Since it's been through so many rounds of review at this point I'm just going to approve it

@sebouh137 sebouh137 added this pull request to the merge queue Jan 23, 2024
Merged via the queue into main with commit a159693 Jan 23, 2024
73 of 75 checks passed
@sebouh137 sebouh137 deleted the sipmzdc branch January 23, 2024 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: calorimetry relates to calorimetry topic: far-forward Far forward reconstruction topic: infrastructure
Projects
Development

Successfully merging this pull request may close these issues.

5 participants