updated with calorimeter, to be checked#11
Open
matclim wants to merge 2 commits intoShipSoft:mainfrom
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughCalorimeter construction moved from fixed sectioning to config-driven layer placement. CalorimeterFactory accepts a config path, resolves and loads Changes
Sequence DiagramsequenceDiagram
participant Client as Client Code
participant Factory as CalorimeterFactory
participant CfgResolver as Config Resolver
participant CfgLoader as Config Loader
participant Builder as buildStack()
participant BarLayer as CaloBarLayer
participant FiberLayer as CaloFibreHPLayer
participant Container as Container Volume
Client->>Factory: build()
activate Factory
Factory->>CfgResolver: resolveCfgPath(m_configPath)
CfgResolver-->>Factory: resolved path (with fallbacks)
Factory->>CfgLoader: readCaloConfig(path)
CfgLoader-->>Factory: CalorimeterConfig
Factory->>Factory: totalStackZ(cfg) / validate fit
loop per module (mx,my)
Factory->>Builder: buildStack(container, cfg, mx, my, offsets)
activate Builder
loop per layer in cfg.layers / cfg.layers2
alt layer = bar
Builder->>BarLayer: place(...)
BarLayer-->>Builder: placed volume
else layer = fiber
Builder->>FiberLayer: build(...)
FiberLayer-->>Builder: built volume
else layer = air
Builder->>Builder: advance Z cursor
end
Builder->>Container: place layer envelope
end
deactivate Builder
end
Factory-->>Client: GeoPhysVol* (container)
deactivate Factory
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
subsystems/Calorimeter/test_calorimeter.cpp (1)
32-39: Make the child-count assertion stronger.
getNChildVols() >= 1only proves the container is non-empty. This refactor can still drop most layers/modules and keep the test green. Please assert the expected count from the stagedcalo.cfg, or at least a lower bound derived frommodule_nx * module_ny * populated_layer_count.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@subsystems/Calorimeter/test_calorimeter.cpp` around lines 32 - 39, Update the test CalorimeterHasChildren to assert a stronger expected child count: instead of CHECK(calo->getNChildVols() >= 1u), compute or hardcode the expected lower bound from the staged configuration and assert equality or a tighter >=, e.g. read module_nx, module_ny and populated_layer_count (or the expected total layers from calo.cfg) and assert calo->getNChildVols() >= module_nx * module_ny * populated_layer_count (or == expected_total_children); modify the test to access SHiPMaterials/CalorimeterFactory configuration values (or parse calo.cfg) and replace the loose check on calo->getNChildVols() with the computed stronger assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/SHiPMaterials.cpp`:
- Around line 194-208: The PVT and Polystyrene materials are being added with
raw atom counts (stoichiometry) instead of normalized mass fractions; update the
GeoMaterial creation for "PVT" and "Polystyrene" to compute mass fractions from
atomic masses (use m_elements["Carbon"] and m_elements["Hydrogen"] to get each
element's atomic weight), compute the molecule molecular weight (e.g., MW =
9*AW_C + 10*AW_H for PVT), then add normalized fractions (count*AW_element / MW)
to the GeoMaterial before lock(), and keep storing them in m_materials["PVT"]
and m_materials["Polystyrene"] as before.
In `@subsystems/Calorimeter/CMakeLists.txt`:
- Around line 34-43: The compile-time fallback CALO_CFG_DEFAULT_PATH currently
embeds the source-tree path but the installed calo.cfg is not probed; update the
build and runtime lookup so installed data is found: add a second compile
definition (or an exported header) that contains the install data directory path
(the ${CMAKE_INSTALL_DATADIR}/SHiPGeometry location) when installing the
Calorimeter target, and modify resolveCfgPath() in CalorimeterFactory.cpp to
check that install-time data-dir path in addition to the working directory and
CALO_CFG_DEFAULT_PATH (use the unique symbol CALO_CFG_DEFAULT_PATH and the
resolveCfgPath function name to locate the places to change). Ensure the
install-time path is only set when performing an install so local development
still falls back to the source-tree path.
In `@subsystems/Calorimeter/src/CalorimeterFactory.cpp`:
- Around line 261-315: The loop in CalorimeterFactory.cpp that iterates
cfg.layers2 accepts codes 1,2,5,6,7,8 but not 3 or 4, causing a runtime throw
while totalStackZ() treats 1..4 as valid; fix by either (A) adding explicit
branches for codes 3 and 4 that create the expected thin-PS scintillator layers
(mirror the pattern used for code 1/2: build an env via makeEnv and call the
appropriate placement/build routine, increment zCursor and indices like sl/gl),
or (B) validate/normalize cfg.layers2 earlier (e.g. in totalStackZ() caller or
CalorimeterFactory constructor) to reject or translate codes 3 and 4 and throw a
clear config error; reference the loop over cfg.layers2, totalStackZ(), makeEnv,
CaloBarLayer::place and CaloFibreHPLayer::build when making the change.
- Around line 83-107: Before placing modules, validate that the calorimeter
stack and tiled footprint fit inside the fixed container: compute total Z extent
using totalStackZ(cfg) and compute full X/Y extents from cfg.module_nx,
cfg.module_ny and effective pitch (use same logic as pitchX/pitchY:
cfg.module_pitch_x_mm>0 ? cfg.module_pitch_x_mm : cfg.plate_xy_mm), then compare
half-extents against s_containerHalfZ and s_containerHalfX/s_containerHalfY
respectively; if any extent exceeds the container, throw a clear runtime_error
(or appropriate config exception) with a descriptive message referencing the
offending dimension and the computed vs allowed size before the loop that calls
buildStack(containerPhys, cfg, ...).
---
Nitpick comments:
In `@subsystems/Calorimeter/test_calorimeter.cpp`:
- Around line 32-39: Update the test CalorimeterHasChildren to assert a stronger
expected child count: instead of CHECK(calo->getNChildVols() >= 1u), compute or
hardcode the expected lower bound from the staged configuration and assert
equality or a tighter >=, e.g. read module_nx, module_ny and
populated_layer_count (or the expected total layers from calo.cfg) and assert
calo->getNChildVols() >= module_nx * module_ny * populated_layer_count (or ==
expected_total_children); modify the test to access
SHiPMaterials/CalorimeterFactory configuration values (or parse calo.cfg) and
replace the loose check on calo->getNChildVols() with the computed stronger
assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 33c8398b-10cc-494d-a272-1a3039478fd2
📒 Files selected for processing (11)
.clang-format.github/dependabot.yml.github/workflows/build-test.yml.gitignore.pre-commit-config.yamlsrc/SHiPGeometry.cppsrc/SHiPMaterials.cppsubsystems/Calorimeter/CMakeLists.txtsubsystems/Calorimeter/include/Calorimeter/CalorimeterFactory.hsubsystems/Calorimeter/src/CalorimeterFactory.cppsubsystems/Calorimeter/test_calorimeter.cpp
💤 Files with no reviewable changes (5)
- .github/dependabot.yml
- .clang-format
- .gitignore
- .github/workflows/build-test.yml
- .pre-commit-config.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@subsystems/Calorimeter/src/CalorimeterFactory.cpp`:
- Around line 185-197: layerId is reset to 0 inside buildStack(), so
GeoIdentifierTag instances created by makeEnv are duplicated across modules
(mx,my) when all layers are placed under the same container; change the code so
identifiers are globally unique per module by either (A) making a per-module
parent GeoPhysVol and attach layers to that parent instead of the shared
container, or (B) incorporate module coordinates into the identifier before
creating GeoIdentifierTag (e.g. compute a base offset using mx,my and add local
layerId), updating references in build(), buildStack(), makeEnv, and any code
that reads GeoIdentifierTag so it expects the encoded identifier; ensure layerId
incrementing still produces unique values and apply same fix to the other
occurrence around lines ~293-296.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0173ed9e-1d2f-4c37-94a7-31da7b805cc1
📒 Files selected for processing (3)
src/SHiPMaterials.cppsubsystems/Calorimeter/CMakeLists.txtsubsystems/Calorimeter/src/CalorimeterFactory.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/SHiPMaterials.cpp
- subsystems/Calorimeter/CMakeLists.txt
Comment on lines
+185
to
+197
| int layerId = 0; // monotonic identifier used for GeoIdentifierTag | ||
|
|
||
| // Lambda: wrap a layer in a named air-box envelope inside the container | ||
| auto makeEnv = [&](const std::string& name, | ||
| double halfZ, | ||
| double zCenter) -> GeoPhysVol* | ||
| { | ||
| auto* s = new GeoBox(0.5*plateXY, 0.5*plateXY, halfZ); | ||
| auto* l = new GeoLogVol(name + "_env", s, airMat); | ||
| auto* p = new GeoPhysVol(l); | ||
| container->add(new GeoNameTag(name.c_str())); | ||
| container->add(new GeoIdentifierTag(layerId++)); | ||
| container->add(new GeoTransform(GeoTrf::Translate3D(offX, offY, zCenter))); |
There was a problem hiding this comment.
Make GeoIdentifierTags unique across modules.
build() places every module's layers directly under the same container, but layerId is reset to 0 on each buildStack() call. That means sibling volumes from different (mx, my) modules will reuse the same identifiers, so identifier-based lookup cannot distinguish modules reliably. Please either introduce a per-module parent volume or encode (mx, my, localLayer) into the tag before attaching children to container.
Also applies to: 293-296
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@subsystems/Calorimeter/src/CalorimeterFactory.cpp` around lines 185 - 197,
layerId is reset to 0 inside buildStack(), so GeoIdentifierTag instances created
by makeEnv are duplicated across modules (mx,my) when all layers are placed
under the same container; change the code so identifiers are globally unique per
module by either (A) making a per-module parent GeoPhysVol and attach layers to
that parent instead of the shared container, or (B) incorporate module
coordinates into the identifier before creating GeoIdentifierTag (e.g. compute a
base offset using mx,my and add local layerId), updating references in build(),
buildStack(), makeEnv, and any code that reads GeoIdentifierTag so it expects
the encoded identifier; ensure layerId incrementing still produces unique values
and apply same fix to the other occurrence around lines ~293-296.
Author
|
I don't understand the pre-commit ci error ? |
Contributor
Because you deleted the pre-commit config file (and a bunch of other files!) in your PR. Please revert these unrelated changes. I'll review the calorimeter changes tomorrow. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@subsystems/Calorimeter/src/CalorimeterFactory.cpp`:
- Around line 269-271: The ECAL branch for code == 8 updates zCursor but does
not increment gl, while the HCAL branch increments gl; make the behavior
explicit and consistent: either add "++gl" immediately after "zCursor +=
airGapZ;" in the ECAL branch to match HCAL, or (if air gaps intentionally count
as geometry layers only for HCAL) add a short clarifying comment in both
branches explaining the difference; update the code paths handling code == 8
(zCursor, airGapZ, gl) in CalorimeterFactory.cpp accordingly so the intent is
unambiguous.
- Around line 185-200: layerId is reset per buildStack(), causing duplicate
GeoIdentifierTag values across modules; modify the code so identifiers are
globally unique by either (A) making layerId a persistent counter (e.g. a member
or static counter used by buildStack()) or (B) encoding module indices into the
id when creating GeoIdentifierTag (e.g. compute id = mx*<scale1> + my*<scale2> +
layerId where <scale1>/<scale2> are chosen to avoid collisions); update the
makeEnv lambda to use the new id calculation or the shared counter when calling
container->add(new GeoIdentifierTag(...)) and keep GeoNameTag usage (mtag)
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4ed00c30-7d24-4015-87a0-de097a6e2244
📒 Files selected for processing (6)
src/SHiPGeometry.cppsrc/SHiPMaterials.cppsubsystems/Calorimeter/CMakeLists.txtsubsystems/Calorimeter/include/Calorimeter/CalorimeterFactory.hsubsystems/Calorimeter/src/CalorimeterFactory.cppsubsystems/Calorimeter/test_calorimeter.cpp
✅ Files skipped from review due to trivial changes (2)
- src/SHiPGeometry.cpp
- src/SHiPMaterials.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- subsystems/Calorimeter/CMakeLists.txt
Comment on lines
+185
to
+200
| int layerId = 0; // monotonic identifier used for GeoIdentifierTag | ||
|
|
||
| // Lambda: wrap a layer in a named air-box envelope inside the container | ||
| auto makeEnv = [&](const std::string& name, | ||
| double halfZ, | ||
| double zCenter) -> GeoPhysVol* | ||
| { | ||
| auto* s = new GeoBox(0.5*plateXY, 0.5*plateXY, halfZ); | ||
| auto* l = new GeoLogVol(name + "_env", s, airMat); | ||
| auto* p = new GeoPhysVol(l); | ||
| container->add(new GeoNameTag(name.c_str())); | ||
| container->add(new GeoIdentifierTag(layerId++)); | ||
| container->add(new GeoTransform(GeoTrf::Translate3D(offX, offY, zCenter))); | ||
| container->add(p); | ||
| return p; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if GeoIdentifierTag-based lookup is used elsewhere in the codebase
rg -n 'GeoIdentifierTag|getChildVol.*[Ii]d|findChildVolById' --type=cppRepository: ShipSoft/Geometry
Length of output: 4179
GeoIdentifierTag values are not unique across modules.
layerId is reset to 0 at the start of each buildStack() call (line 185). Since all modules place layers directly into the same container, sibling volumes from different (mx, my) modules will have identical identifier values (0, 1, 2, ...). This pattern is inconsistent with other factories in the codebase (e.g., TargetFactory, MagnetFactory) which use unique sequential identifiers.
The volumes do have unique names via the mtag suffix (e.g., _MX0Y0, _MX1Y0), providing a fallback for name-based lookups. However, to maintain consistency with the rest of the codebase and ensure identifier-based lookup works correctly if used downstream, consider encoding module indices into the identifier (e.g., mx * 100 + my * 10 + layerId) or grouping each module's layers under a dedicated parent volume.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@subsystems/Calorimeter/src/CalorimeterFactory.cpp` around lines 185 - 200,
layerId is reset per buildStack(), causing duplicate GeoIdentifierTag values
across modules; modify the code so identifiers are globally unique by either (A)
making layerId a persistent counter (e.g. a member or static counter used by
buildStack()) or (B) encoding module indices into the id when creating
GeoIdentifierTag (e.g. compute id = mx*<scale1> + my*<scale2> + layerId where
<scale1>/<scale2> are chosen to avoid collisions); update the makeEnv lambda to
use the new id calculation or the shared counter when calling container->add(new
GeoIdentifierTag(...)) and keep GeoNameTag usage (mtag) unchanged.
Comment on lines
+269
to
+271
| } else if (code == 8) { | ||
| zCursor += airGapZ; | ||
|
|
There was a problem hiding this comment.
Inconsistent gl increment for air gap (code 8) between ECAL and HCAL.
In the ECAL section (line 270), code 8 only advances zCursor:
} else if (code == 8) {
zCursor += airGapZ;In the HCAL section (line 351), code 8 also increments gl:
} else if (code == 8) {
zCursor += airGapZ; ++gl;If this is intentional (air gaps count as "geometry layers" in HCAL but not in ECAL), a brief comment would clarify the design. Otherwise, consider aligning the behavior.
Also applies to: 350-352
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@subsystems/Calorimeter/src/CalorimeterFactory.cpp` around lines 269 - 271,
The ECAL branch for code == 8 updates zCursor but does not increment gl, while
the HCAL branch increments gl; make the behavior explicit and consistent: either
add "++gl" immediately after "zCursor += airGapZ;" in the ECAL branch to match
HCAL, or (if air gaps intentionally count as geometry layers only for HCAL) add
a short clarifying comment in both branches explaining the difference; update
the code paths handling code == 8 (zCursor, airGapZ, gl) in
CalorimeterFactory.cpp accordingly so the intent is unambiguous.
Contributor
These files all seem to be missing. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please check that it works as expected, tests are passed but I am not sure what they entail
Summary by CodeRabbit
New Features
Tests
Chores