Implement H4, H3 and S2 staggering in HEXPLIT#2589
Conversation
github-actions
bot
added
topic: calorimetry
removed "sin(pi/3)" thing that was there from the hexagonal version of the ZDC
for more information, see https://pre-commit.ci
comment out some lines that were commented out before
reverted formatting of commented-out lines
Fix issue where "OVERLAP" is not defined
for more information, see https://pre-commit.ci
replaced ; with ,
for more information, see https://pre-commit.ci
don't use arrays that are assigned a length at runtime. Better to use the maximum value that could be considered for any known staggering pattern (12).
for more information, see https://pre-commit.ci
This PR applies the include-what-you-use fixes as suggested by https://github.com/eic/EICrecon/actions/runs/23884066302. Please merge this PR into the branch `hexplit_s2` to resolve failures in PR #2589. Auto-generated by [create-pull-request][1] [1]: https://github.com/peter-evans/create-pull-request Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
adjusted thresholds for the clustering
|
|
The campaign benchmark was fixed in eic/simulation_campaign_hepmc3#88. For this PR, since this is not a critical subsystem, we may not need to require a perfect backward compatibility (which is is always welcome in any case), but the minimal goals would be to at least:
|
@veprbl Hi Dmitry, thanks for the info. This doesn't cause crashes when running the new EICrecon with the previous version of epic, however this does affect the results from the EICrecon, which can cause the analysis scripts in the benchmarks to fail. Essentially, clusters can still be found, but since the subcell splitting of the HEXPLIT algorithm requires the geometry to match the one used in the simulation, it will distribute the energy in a given cell evenly (since it doesn't know how the cells in different layers are supposed to overlap). This causes any analysis that requires multiple clusters per event to fail because the clusters are getting merged together, causing the analysis step of the benchmarks to crash (in the case of the zdc_pi0 benchmark), or produce garbage results. I have locally checked that the zdc_pi0 benchmark works on my laptop with both repositories on the correct branches. So, all three of these criteria appear to be fulfilled. There will be followup work that will need to be done, due to new geometry, but that can be done in future pull requests. |
veprbl
left a comment
veprbl
left a comment
There was a problem hiding this comment.
Thank you for checking the benchmarks. I have couple of comments.
|
|
||
| ParameterRef<std::string> m_energyWeight{this, "energyWeight", config().energyWeight}; | ||
| ParameterRef<double> m_samplingFraction{this, "samplingFraction", config().sampFrac}; | ||
| //ParameterRef<double> m_samplingFraction{this, "samplingFraction", config().sampFrac}; |
There was a problem hiding this comment.
Removing the sampling fraction option doesn't seem like a good idea. It's a useful option.
Would it work to grab the detector auto detector = app->GetService<DD4hep_service>()->detector(); and then call _toDouble just in ZDC.cc?
There was a problem hiding this comment.
I'll fix this, thanks. I was trying to have it be able to scale the sampling fraction based on the thickness of the tiles for backwards compatibility, but what you suggested is probably a better way to do this.
| .Emin_in_MIPs = 0.5, | ||
| .delta_in_MIPs = 0.01, | ||
| .tmax = 269 * dd4hep::ns, | ||
| .stag_type = HEXPLITConfig::StaggerType::S2, |
There was a problem hiding this comment.
If you want, you could infer staggering type from segmentation, e.g.:
Would that make it also backwards compatible?
restored sampling fraction as a parameter
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
4 participants
Briefly, what does this PR introduce?
Update the HEXPLIT algorithm to be able to handle square cells, which are used in the latest version of the ZDC geometry. Also update the parameters in the ZDC reconstruction to match the geometry (which also has thicker cells, so larger sampling fraction and larger MIP value.
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?
Users should use the square-cell version of the ZDC geometry in the epic repository
Does this PR change default behavior?
Yes. Presumes that the ZDC has square cells, and runs the square-cell version of HEXPLIT.