Conversation
|
☔ The build tests failed for e451291.
N.B. These results were obtained from a build of this Pull Request at e451291 after being merged into the base branch at acfae4f. For more information, please check the job page here. |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for e451291: build (Build queue - API unavailable) |
|
☀️ The build tests passed at e451291.
N.B. These results were obtained from a build of this Pull Request at e451291 after being merged into the base branch at 9fbff5d. For more information, please check the job page here. |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for b1b7f1c: build (Build queue - API unavailable) |
There was a problem hiding this comment.
Pull request overview
This PR adds new virtual detectors for electromagnetic calorimeter (EMC) studies and tracker front-end board (FEB) measurements in the Mu2e detector simulation.
Changes:
- Added three new EMC virtual detector IDs (EMC_Source, EMC_Source2, EMC_0_Front) and 19 Tracker FEB virtual detector IDs (Tracker_FEB_0-18_SurfIn) to the VirtualDetectorId enumeration
- Implemented construction logic for these new virtual detectors in both VirtualDetectorMaker.cc and constructVirtualDetectors.cc
- Added configuration parameters for EMC virtual detector z-positions to the geometry file
- Fixed uninitialized variable warnings by adding default initializers to opaz0, opaz1, opari0, opari1 variables
- Refactored disk calorimeter virtual detector code formatting and uncommented previously disabled tracker virtual detector code
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| DataProducts/inc/VirtualDetectorId.hh | Added 22 new virtual detector enum values and corresponding string names, plus isFEBTracker() helper method |
| GeometryService/src/VirtualDetectorMaker.cc | Uncommented and updated tracker virtual detector creation logic; added EMC_Source, EMC_Source2, and EMC_0_Front virtual detector definitions; uncommented solenoidOffset variable |
| Mu2eG4/src/constructVirtualDetectors.cc | Fixed uninitialized variable warnings; added construction logic for new EMC and Tracker FEB virtual detectors; refactored calorimeter virtual detector code formatting; added isDumbbell configuration logic for parent volume selection |
| Mu2eG4/geom/geom_run1_b_v01.txt | Added configuration parameters for EMC virtual detector z-positions (zEMCSourceInMu2e, zEMCSource2InMu2e, zEMC0Front) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ++vdIdDiskEdge; | ||
|
|
||
| ++vdIdFEBSurf; | ||
| //needed to maintain the consistence with the numbering scheme |
There was a problem hiding this comment.
Spelling error in comment: 'consistence' should be 'consistency'.
| //needed to maintain the consistence with the numbering scheme | |
| //needed to maintain the consistency with the numbering scheme |
| false); | ||
|
|
||
| // doSurfaceCheck && checkForOverlaps(vd.physical, _config, verbosityLevel>0); | ||
| doSurfaceCheck && checkForOverlaps(vd.physical, _config, true); |
There was a problem hiding this comment.
This overlap check uses hardcoded 'true' for the verbosity parameter instead of 'verbosityLevel>0' which is used consistently elsewhere in the file (see lines 1947, 2011, 2076, etc.). This forces verbose output regardless of the configured verbosity level. Either this should be changed to match the pattern used elsewhere, or if the forced verbosity is intentional, a comment explaining why should be added.
| doSurfaceCheck && checkForOverlaps(vd.physical, _config, true); | |
| doSurfaceCheck && checkForOverlaps(vd.physical, _config, verbosityLevel>0); |
| VolumeInfo const & parent = ( _config.getBool("isDumbbell",false) ) ? | ||
| _helper->locateVolInfo(theDS3) : |
There was a problem hiding this comment.
The parent volume selection logic is redundant. This code is inside a block that only executes when 'isDumbbell' is false (line 1890), so the ternary operator on lines 1925-1927 will always evaluate to false and always select DS2Vacuum. The check for isDumbbell is redundant here. Either remove the outer 'if ( !_config.getBool("isDumbbell",false) )' condition, or simplify the parent volume selection to just use DS2Vacuum directly without the ternary operator.
| VolumeInfo const & parent = ( _config.getBool("isDumbbell",false) ) ? | |
| _helper->locateVolInfo(theDS3) : | |
| VolumeInfo const & parent = |
| std::string theDS3("DS3Vacuum"); | ||
| if ( _config.getBool("inGaragePosition",false) ) theDS3 = "garageFakeDS3Vacuum"; | ||
|
|
||
| VolumeInfo const & parent = ( _config.getBool("isDumbbell",false) ) ? | ||
| _helper->locateVolInfo(theDS3) : | ||
| _helper->locateVolInfo("DS2Vacuum"); //DS3Vacuum to move the targets |
There was a problem hiding this comment.
The parent volume selection logic is redundant. This code is inside a block that only executes when 'isDumbbell' is false (line 1953), so the ternary operator on lines 1989-1991 will always evaluate to false and always select DS2Vacuum. The check for isDumbbell is redundant here. Either remove the outer 'if ( !_config.getBool("isDumbbell",false) )' condition, or simplify the parent volume selection to just use DS2Vacuum directly without the ternary operator.
| std::string theDS3("DS3Vacuum"); | |
| if ( _config.getBool("inGaragePosition",false) ) theDS3 = "garageFakeDS3Vacuum"; | |
| VolumeInfo const & parent = ( _config.getBool("isDumbbell",false) ) ? | |
| _helper->locateVolInfo(theDS3) : | |
| _helper->locateVolInfo("DS2Vacuum"); //DS3Vacuum to move the targets | |
| VolumeInfo const & parent = _helper->locateVolInfo("DS2Vacuum"); |
| if ( _config.getBool("hasTracker",false) ) { | ||
| // the radius of tracker support | ||
| Tracker const & tracker = *(GeomHandle<Tracker>()); | ||
| double orvd = tracker.g4Tracker()->getSupportParams().getTubsParams().outerRadius()-20.1; // Avoid tracker support beam | ||
| //double irvd = tracker.g4Tracker()->getSupportParams().getTubsParams().innerRadius(); | ||
| double irvd = tracker.g4Tracker()->mother().tubsParams().innerRadius(); | ||
|
|
||
| cout << "TRACKER FEB VD OUTER INNER RADIUS " << orvd << " " << irvd << endl; | ||
| TubsParams vdParamsTracker(irvd,orvd,vdHalfLength); | ||
| cout << "Tracker_FEB_SurfIn parameters: " << vdParamsTracker << endl; | ||
| VolumeInfo const & parent = _helper->locateVolInfo("TrackerMother"); | ||
| for ( int ipln=0; ipln<StrawId::_nplanes+1; ipln+=2 ){ | ||
| // placing virtual detectors before FEB of tracker stations | ||
| vdId = VirtualDetectorId::Tracker_FEB_0_SurfIn+ipln/2; | ||
| if( vdg->exist(vdId) ) { | ||
| if ( verbosityLevel > 0) { | ||
| cout << __func__ << " constructing " << VirtualDetector::volumeName(vdId) << endl; | ||
| } | ||
| CLHEP::Hep3Vector vdPos = vdg->getGlobal(vdId)-parent.centerInMu2e(); | ||
| cout << "Tracker_FEB_" << ipln/2 << "_SurfIn: " << vdPos << " " << vdg->getLocal(vdId) << endl; | ||
|
|
||
| VolumeInfo vd = nestTubs( VirtualDetector::volumeName(vdId), | ||
| vdParamsTracker, downstreamVacuumMaterial, 0, | ||
| vdPos, | ||
| parent, | ||
| vdId, vdIsVisible, G4Color::Red(), vdIsSolid, | ||
| forceAuxEdgeVisible, | ||
| placePV, | ||
| false); | ||
|
|
||
| // doSurfaceCheck && checkForOverlaps(vd.physical, _config, verbosityLevel>0); | ||
| doSurfaceCheck && checkForOverlaps(vd.physical, _config, true); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Inconsistent condition between VirtualDetectorMaker.cc and constructVirtualDetectors.cc. In VirtualDetectorMaker.cc (line 259), Tracker_FEB virtual detectors are only created when 'TrackerHasBrassRings' is true. However, in constructVirtualDetectors.cc (line 2081), the code attempts to construct them when only 'hasTracker' is true. While the vdg->exist(vdId) check on line 2095 may prevent errors, this inconsistency could lead to confusion. Consider adding the same TrackerHasBrassRings check here for clarity and to match the logic in VirtualDetectorMaker.cc.
| VolumeInfo vdInfo = nestTubs(VirtualDetector::volumeName(vdIdFEBSurf), | ||
| vdParamsFrontFEB,downstreamVacuumMaterial,0,posFrontFEB,caloFEBParent,vdIdFEBSurf,vdIsVisible,G4Color::Red(),vdIsSolid,forceAuxEdgeVisible,placePV,false); | ||
| ++vdIdFEBSurf; | ||
|
|
||
| VolumeInfo vdInfo2 = nestTubs(VirtualDetector::volumeName(vdIdFEBSurf),vdParamsFrontFEB,downstreamVacuumMaterial,0,posBackFEB,caloFEBParent,vdIdFEBSurf,vdIsVisible,G4Color::Red(),vdIsSolid,forceAuxEdgeVisible,placePV,false); | ||
|
|
||
|
|
||
| ++vdIdFEBSurf; | ||
|
|
||
| doSurfaceCheck && checkForOverlaps(vdInfo.physical, _config, verbosityLevel>0); | ||
| doSurfaceCheck && checkForOverlaps(vdInfo2.physical, _config, verbosityLevel>0); | ||
| } | ||
|
|
||
| if( vdg->exist(vdIdFEBEdge)){ | ||
| VolumeInfo vdInfo = nestTubs(VirtualDetector::volumeName(vdIdFEBEdge), | ||
| vdParamsInnerFEB,downstreamVacuumMaterial,0,posInnerFEB,caloFEBParent,vdIdFEBEdge,vdIsVisible,G4Color::Red(),vdIsSolid,forceAuxEdgeVisible,placePV,false); |
There was a problem hiding this comment.
Line 1302 and 1316 have incorrect indentation. The continuation of the nestTubs function call should be indented properly, not starting at column 0. This makes the code hard to read and breaks visual alignment with the rest of the code.
| vd->addVirtualDetector( VirtualDetectorId::EMC_Source2, | ||
| ds2centerInMu2e, 0, posEMCSource2); | ||
| // VD EMC_0_Front is placed in front of EMC Disk 0 | ||
| const double zVDEMC0Front = c.getDouble("zEMC0Front",5830.); |
There was a problem hiding this comment.
The default value for 'zEMC0Front' is inconsistent between the geometry configuration file (8695.0) and the code default (5830.0). This could lead to unexpected behavior if the configuration parameter is not explicitly set. The defaults should match to ensure consistent behavior.
| const double zVDEMC0Front = c.getDouble("zEMC0Front",5830.); | |
| const double zVDEMC0Front = c.getDouble("zEMC0Front",8695.0); |
| std::string theDS3("DS3Vacuum"); | ||
| if ( _config.getBool("inGaragePosition",false) ) theDS3 = "garageFakeDS3Vacuum"; | ||
|
|
||
| VolumeInfo const & parent = ( _config.getBool("isDumbbell",false) ) ? | ||
| _helper->locateVolInfo(theDS3) : | ||
| _helper->locateVolInfo("DS2Vacuum"); //DS3Vacuum to move the targets |
There was a problem hiding this comment.
The parent volume selection logic is redundant. This code is inside a block that only executes when 'isDumbbell' is false (line 2017), so the ternary operator on lines 2054-2056 will always evaluate to false and always select DS2Vacuum. The check for isDumbbell is redundant here. Either remove the outer 'if ( !_config.getBool("isDumbbell",false) )' condition, or simplify the parent volume selection to just use DS2Vacuum directly without the ternary operator.
| std::string theDS3("DS3Vacuum"); | |
| if ( _config.getBool("inGaragePosition",false) ) theDS3 = "garageFakeDS3Vacuum"; | |
| VolumeInfo const & parent = ( _config.getBool("isDumbbell",false) ) ? | |
| _helper->locateVolInfo(theDS3) : | |
| _helper->locateVolInfo("DS2Vacuum"); //DS3Vacuum to move the targets | |
| VolumeInfo const & parent = | |
| _helper->locateVolInfo("DS2Vacuum"); // DS3Vacuum to move the targets |
| vdId = VirtualDetectorId::EMC_Source; | ||
|
|
||
| if ( !_config.getBool("isDumbbell",false) ){ | ||
| double Ravr = ds->rIn1(); | ||
|
|
||
| if ( _config.getBool("hasTSdA",false) ) { | ||
| Ravr = _config.getDouble("TSdA.rFactorForVDs"); | ||
| } | ||
|
|
||
| bool opaflag = false; | ||
| double opaz0=0., opaz1=0., opari0=0., opari1=0.; | ||
| if ( _config.getBool("hasProtonAbsorber", true) ) { | ||
| GeomHandle<MECOStyleProtonAbsorber> pageom; | ||
| if ( pageom->isAvailable(ProtonAbsorberId::opabs1) ) { | ||
| opaflag = true; | ||
| MECOStyleProtonAbsorberPart opa = pageom->part(2); | ||
| opaz0 = opa.center().z()-opa.halfLength(); | ||
| opaz1 = opa.center().z()+opa.halfLength(); | ||
| opari0 = opa.innerRadiusAtStart(); | ||
| opari1 = opa.innerRadiusAtEnd(); | ||
| } | ||
| } | ||
| if( vdg->exist(vdId) ) { | ||
| cout << __func__ << " constructing " << VirtualDetector::volumeName(vdId) << endl; | ||
|
|
||
| double zvd = vdg->getGlobal(vdId).z(); | ||
| if (opaflag) { | ||
| Ravr = (opari1 - opari0)/(opaz1 - opaz0) * (zvd - opaz0) + opari0; | ||
| } | ||
| double rvd = Ravr - 100.0; | ||
| cout << __func__ << " " << VirtualDetector::volumeName(vdId) << | ||
| " z, r : " << zvd << ", " << rvd << endl; | ||
|
|
||
| TubsParams vdParamsTarget(0.,rvd,vdHalfLength); | ||
| std::string theDS3("DS3Vacuum"); | ||
| if ( _config.getBool("inGaragePosition",false) ) theDS3 = "garageFakeDS3Vacuum"; | ||
|
|
||
| VolumeInfo const & parent = ( _config.getBool("isDumbbell",false) ) ? | ||
| _helper->locateVolInfo(theDS3) : | ||
| _helper->locateVolInfo("DS2Vacuum"); //DS3Vacuum to move the targets | ||
|
|
||
| if (verbosityLevel >0) { | ||
| cout << __func__ << " " << VirtualDetector::volumeName(vdId) << " Z offset in Mu2e : " << | ||
| zvd << endl; | ||
| cout << __func__ << " " << VirtualDetector::volumeName(vdId) << " Z extent in Mu2e : " << | ||
| zvd - vdHalfLength << ", " << zvd + vdHalfLength << endl; | ||
| } | ||
|
|
||
| VolumeInfo vd = nestTubs( VirtualDetector::volumeName(vdId), | ||
| vdParamsTarget, downstreamVacuumMaterial, 0, | ||
| vdg->getLocal(vdId), | ||
| parent, | ||
| vdId, | ||
| vdIsVisible, | ||
| G4Color::Red(), vdIsSolid, | ||
| forceAuxEdgeVisible, | ||
| placePV, | ||
| false); | ||
|
|
||
| doSurfaceCheck && checkForOverlaps(vd.physical, _config, verbosityLevel>0); | ||
| } | ||
| } | ||
|
|
||
| vdId = VirtualDetectorId::EMC_Source2; | ||
|
|
||
| if ( !_config.getBool("isDumbbell",false) ){ | ||
| double Ravr = ds->rIn1(); | ||
|
|
||
| if ( _config.getBool("hasTSdA",false) ) { | ||
| Ravr = _config.getDouble("TSdA.rFactorForVDs"); | ||
| } | ||
|
|
||
| bool opaflag = false; | ||
| double opaz0=0., opaz1=0., opari0=0., opari1=0.; | ||
| if ( _config.getBool("hasProtonAbsorber", true) ) { | ||
| GeomHandle<MECOStyleProtonAbsorber> pageom; | ||
| if ( pageom->isAvailable(ProtonAbsorberId::opabs1) ) { | ||
| opaflag = true; | ||
| MECOStyleProtonAbsorberPart opa = pageom->part(2); | ||
| opaz0 = opa.center().z()-opa.halfLength(); | ||
| opaz1 = opa.center().z()+opa.halfLength(); | ||
| opari0 = opa.innerRadiusAtStart(); | ||
| opari1 = opa.innerRadiusAtEnd(); | ||
| } | ||
| } | ||
| if( vdg->exist(vdId) ) { | ||
| cout << __func__ << " constructing " << VirtualDetector::volumeName(vdId) << endl; | ||
|
|
||
| double zvd = vdg->getGlobal(vdId).z(); | ||
| if (opaflag) { | ||
| Ravr = (opari1 - opari0)/(opaz1 - opaz0) * (zvd - opaz0) + opari0; | ||
| } | ||
| double rvd = Ravr - 100.0; | ||
|
|
||
| cout << __func__ << " " << VirtualDetector::volumeName(vdId) << | ||
| " z, r : " << zvd << ", " << rvd << endl; | ||
|
|
||
| TubsParams vdParamsTarget(0.,rvd,vdHalfLength); | ||
| std::string theDS3("DS3Vacuum"); | ||
| if ( _config.getBool("inGaragePosition",false) ) theDS3 = "garageFakeDS3Vacuum"; | ||
|
|
||
| VolumeInfo const & parent = ( _config.getBool("isDumbbell",false) ) ? | ||
| _helper->locateVolInfo(theDS3) : | ||
| _helper->locateVolInfo("DS2Vacuum"); //DS3Vacuum to move the targets | ||
|
|
||
| if (verbosityLevel >0) { | ||
| cout << __func__ << " " << VirtualDetector::volumeName(vdId) << " Z offset in Mu2e : " << | ||
| zvd << endl; | ||
| cout << __func__ << " " << VirtualDetector::volumeName(vdId) << " Z extent in Mu2e : " << | ||
| zvd - vdHalfLength << ", " << zvd + vdHalfLength << endl; | ||
| } | ||
|
|
||
| VolumeInfo vd = nestTubs( VirtualDetector::volumeName(vdId), | ||
| vdParamsTarget, downstreamVacuumMaterial, 0, | ||
| vdg->getLocal(vdId), | ||
| parent, | ||
| vdId, | ||
| vdIsVisible, | ||
| G4Color::Red(), vdIsSolid, | ||
| forceAuxEdgeVisible, | ||
| placePV, | ||
| false); | ||
|
|
||
| doSurfaceCheck && checkForOverlaps(vd.physical, _config, verbosityLevel>0); | ||
| } | ||
| } | ||
|
|
||
| vdId = VirtualDetectorId::EMC_0_Front; | ||
|
|
||
| if ( !_config.getBool("isDumbbell",false) ){ | ||
| double Ravr = ds->rIn1(); | ||
|
|
||
| if ( _config.getBool("hasTSdA",false) ) { | ||
| Ravr = _config.getDouble("TSdA.rFactorForVDs"); | ||
| } | ||
|
|
||
| bool opaflag = false; | ||
| double opaz0=0., opaz1=0., opari0=0., opari1=0.; | ||
| if ( _config.getBool("hasProtonAbsorber", true) ) { | ||
| GeomHandle<MECOStyleProtonAbsorber> pageom; | ||
| if ( pageom->isAvailable(ProtonAbsorberId::opabs1) ) { | ||
| opaflag = true; | ||
| MECOStyleProtonAbsorberPart opa = pageom->part(2); | ||
| opaz0 = opa.center().z()-opa.halfLength(); | ||
| opaz1 = opa.center().z()+opa.halfLength(); | ||
| opari0 = opa.innerRadiusAtStart(); | ||
| opari1 = opa.innerRadiusAtEnd(); | ||
| } | ||
| } | ||
| if( vdg->exist(vdId) ) { | ||
| cout << __func__ << " constructing " << VirtualDetector::volumeName(vdId) << endl; | ||
|
|
||
| double zvd = vdg->getGlobal(vdId).z(); | ||
| if (opaflag) { | ||
| Ravr = (opari1 - opari0)/(opaz1 - opaz0) * (zvd - opaz0) + opari0; | ||
| } | ||
| double rvd = Ravr - 100.0; | ||
|
|
||
| cout << __func__ << " " << VirtualDetector::volumeName(vdId) << | ||
| " z, r : " << zvd << ", " << rvd << endl; | ||
|
|
||
|
|
||
| TubsParams vdParamsTarget(0.,rvd,vdHalfLength); | ||
| std::string theDS3("DS3Vacuum"); | ||
| if ( _config.getBool("inGaragePosition",false) ) theDS3 = "garageFakeDS3Vacuum"; | ||
|
|
||
| VolumeInfo const & parent = ( _config.getBool("isDumbbell",false) ) ? | ||
| _helper->locateVolInfo(theDS3) : | ||
| _helper->locateVolInfo("DS2Vacuum"); //DS3Vacuum to move the targets | ||
|
|
||
| if (verbosityLevel >0) { | ||
| cout << __func__ << " " << VirtualDetector::volumeName(vdId) << " Z offset in Mu2e : " << | ||
| zvd << endl; | ||
| cout << __func__ << " " << VirtualDetector::volumeName(vdId) << " Z extent in Mu2e : " << | ||
| zvd - vdHalfLength << ", " << zvd + vdHalfLength << endl; | ||
| } | ||
|
|
||
| VolumeInfo vd = nestTubs( VirtualDetector::volumeName(vdId), | ||
| vdParamsTarget, downstreamVacuumMaterial, 0, | ||
| vdg->getLocal(vdId), | ||
| parent, | ||
| vdId, | ||
| vdIsVisible, | ||
| G4Color::Red(), vdIsSolid, | ||
| forceAuxEdgeVisible, | ||
| placePV, | ||
| false); | ||
|
|
||
| doSurfaceCheck && checkForOverlaps(vd.physical, _config, verbosityLevel>0); | ||
| } | ||
| } |
There was a problem hiding this comment.
The code blocks for constructing EMC_Source, EMC_Source2, and EMC_0_Front virtual detectors contain significant duplication. The three blocks (lines 1888-1949, 1951-2013, and 2015-2078) are nearly identical except for the vdId being constructed. Consider extracting this logic into a helper function that takes the vdId as a parameter to reduce code duplication and improve maintainability.
| opaz1 = opa.center().z()+opa.halfLength(); | ||
| opari0 = opa.innerRadiusAtStart(); | ||
| opari1 = opa.innerRadiusAtEnd(); | ||
| } |
There was a problem hiding this comment.
Inconsistent indentation: this block uses single space indentation while the surrounding code at line 1902 uses double space indentation. The indentation should be consistent with the rest of the block.
| } | |
| } |
|
☀️ The build tests passed at b1b7f1c.
N.B. These results were obtained from a build of this Pull Request at b1b7f1c after being merged into the base branch at 047cad6. For more information, please check the job page here. |
No description provided.