Improvements to sEPD software #3828
Conversation
|
The changes to calotowercalib seem largely unnecessary and undesirable. But can you just use the convention for the CDB tree name already in place? Putting in this code just because this is the BDC tree already in the CDB is just laziness and add to code bloat. I fully support integrating the EPD into the main calo software but the whole point is to not use a bunch of flags for EPD when avoidable. if I am looking at this right you should only need to change a line or two of calotowercalib. |
|
@bseidlit Agreed. I have reverted those changes. I will fix the calibration file to use the expected convention |
|
I have some significant technical concerns about the proposed changes to retire EpdReco and replace it with sEPDGeomMapping: Architectural Issues: The current approach introduces unnecessary complexity by duplicating geometry calculations. The workflow now becomes: copy phi, r, z values from EpdReco into sEPDGeomMapping with hardcoded phi values → convert to Cartesian coordinates → store in RawTowerGeomv1 → recalculate phi in downstream EventPlaneReco code. This circular conversion adds computational overhead and potential precision loss without clear benefit. Geometric Modeling Concerns: The EPD detector has a disk geometry, which differs fundamentally from cylindrical calorimeter geometries. The existing EpdGeom class was specifically designed to handle the r, φ, z coordinate system appropriate for disk detectors. Forcing EPD into the calorimeter tower model is precisely why the geometry functionality had to be duplicated from EpdReco into the new sEPDGeomMapping class - it's an indication that we're working against the natural geometry rather than with it. Simulation Impact: The proposed changes would retire the EpdGeom class, which is currently used by simulation code. This creates a significant risk of breaking existing simulation workflows that depend on the current geometry implementation. Recommendation: Before proceeding with this major refactoring, I suggest we: |
|
I do not completely disagree with @eumaka --I definitely think that whether the sEPD geometry handling should be done in CaloGeomMapping or in its own, separate module is worth further discussion, since CaloGeomMapping does seem to assume a cylindrical geometry; however, I do think that it's worthwhile to adhere more closely to the conventions set by the calorimeters, which would mean using RawTowerGeom objects when possible. Note that RawTowerGeomv1 itself does not assume anything about the geometry of the detector: it allows us to use cartesian coordinates to locate towers ( ), which should be as inexpensive as storing arm, ring, phi. Either way, we store three values.Regarding the workflow: the first step ("copy phi, r, z values from EpdReco into sEPDGeomMapping with hardcoded phi values") was a part of the process of refactoring itself and will not be done every time the code is run, so this does not add anything to the computational cost. The remaining steps contribute a marginal amount of additional computing time and are done in other calorimeters without issue (for example, the HCal: and the EMCal: ).Regarding the simulation: I do not think that this would be that great of a challenge. There are relatively few places in the simulation code that I can see making use of EpdGeom (specifically here ) although please correct me if I am wrong. |
Build & test reportReport for commit ea6e51026778b9f9c8ffea1adaff0820ddd8db89:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 3d4c104db7ceb62f83147a66212a425fe6ff4149:
Automatically generated by sPHENIX Jenkins continuous integration |
|
The changes to CaloTowerCalib look good to me, even this time saving continue statement, nice! |
|
Also, the converted calibration file that works with CaloTowerCalib is available at /sphenix/user/ecroft/sEPD_condor/calibrations/sEPD_CalibConstants_CDB_Run71504_converted.root. I've tested it with run 71738 and produced a few plots to validate that the N_MIP distributions look like what we expect. |
|
GreatYes, I think we don't want to change the core functionality of the EPD for like 2-3 weeks. Is the code that produces these calibrations on git. If not, you could work on getting that into coresoftware or macros PR'ed and merged b.c. that won't effect the reconstruction but is also necessary. |
largewedge
force-pushed
the
sepd-caloreco-integration
branch
from
77671e3 to
44b52ae
Compare
Build & test reportReport for commit 44b52ae908fe3910a456dd3a18b013dab0cb4b5b:
Automatically generated by sPHENIX Jenkins continuous integration |
3 participants
This PR adds support for applying the sEPD calibration in CaloTowerCalib and for adding the sEPD geometry to the node tree using the RawTowerGeomContainer, in keeping with the conventions followed by calorimeter software.
Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
As discussed at the BulkTG meeting: https://indico.bnl.gov/event/29269/
Note that this requires the calibrations file to use the tower key to index channels.
TODOs (if applicable)
Consider moving sEPDGeomMapping functionality into CaloGeomMapping.
Change sEPD calibration files to store (1/MPV) rather than MPV so that no special cases are needed in CaloTowerCalib.
Links to other PRs in macros and calibration repositories (if applicable)
Corresponding changes in production macros at this PR: sPHENIX-Collaboration/macros#1176