fix(decayvolume): derive helium from the SBT surfaces instead of a ha…#68
Open
matclim wants to merge 5 commits into
Open
fix(decayvolume): derive helium from the SBT surfaces instead of a ha…#68matclim wants to merge 5 commits into
matclim wants to merge 5 commits into
Conversation
…nd-written inset
The helium decay region was sized by an inset expression in DecayVolumeFactory
that duplicated, by hand, the placement rules used by SBTSensorBuilder and
SBTStructureBuilder. The two were not linked, and the copy was wrong in two
ways:
* It assumed the sensors' inner surface was linear in Z. It is not:
placeSideContainer gives each container a flat outer face over the first
zSplitOffset() of every sub-frustum so as to clear the columns, so the free
region is a sawtooth. The helium overlapped the side containers by 122.6 mm.
* It assumed the innermost material in Y was scintillator. It is not: the
top/bottom longitudinal beams straddle the sensor plane (the web is omitted
because it would pass through the cells) and their inner flange hangs below
it, into the decay region. The helium overlapped those flanges by 2.5 mm.
Neither was detectable: OverlapCheck is a subsystem-level bounding-box test and
does not probe siblings inside decay_volume.
Stop maintaining a second model of the geometry. The placement rules now live on
SBTConfig as primitives (sideContainerCentreX, topBottomContainerCentreY,
longBeamCentreY, zSplitOffset, xHalfAt/yHalfAt); both builders place with them,
and the new SBTEnvelope derives the inner free surface from the same
expressions. A change to a placement rule now moves the helium automatically.
The helium becomes a stack of GeoTraps (2 per sub-frustum, 20 total) so it can
follow the sawtooth exactly, and is inset from the true inner surface by
helium_clearance_mm, now 1 um — enough to avoid coincident boundaries, which
Geant4's navigator handles badly, and far below any engineering tolerance.
Verification is now a property, not a formula: SAT (exact for the convex
hexahedra involved) between every helium slab and every SBT volume in the built
tree, asserting both no protrusion and no margin. A parameter sweep repeats it
across 14 SBT configurations, so the guarantee survives re-parameterisation
rather than holding at one lucky point.
Fiducial helium volume: 534.80 -> 485.85 m^3. The old figure included steel.
Closes ShipSoft#58
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe SBT geometry now centralizes placement calculations, derives helium as multiple slabs from the placed SBT envelope, stores the built configuration, validates sensor-clearance constraints, and adds geometric regression tests. ChangesSBT helium envelope
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DecayVolumeFactory
participant SBTStructureBuilder
participant SBTSensorBuilder
participant SBTEnvelope
participant GeoPhysVol
DecayVolumeFactory->>SBTStructureBuilder: build(cfg)
DecayVolumeFactory->>SBTSensorBuilder: build(cfg)
DecayVolumeFactory->>SBTEnvelope: buildHelium(container, helium, cfg)
SBTEnvelope->>SBTEnvelope: compute envelope knots and helium pieces
SBTEnvelope->>GeoPhysVol: place helium GeoTrap slabs
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
🧹 Nitpick comments (1)
subsystems/DecayVolume/include/DecayVolume/SBTConfig.h (1)
129-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUndocumented magic constant
1.0intopBottomAvailX().This block is explicitly documented as the single source of truth for placement math ("do not open-code these expressions anywhere else"), but the
- 1.0here has no comment explaining what it represents (a clearance? rounding margin?). Worth a short inline comment for the next reader who has to trust this number.✏️ Suggested doc comment
- /// Half-extent in X available to top/bottom containers (mm). + /// Half-extent in X available to top/bottom containers (mm). + /// The 1 mm subtracted is a [explain: e.g. fabrication/placement margin]. double topBottomAvailX(double x_half_mm) const { return x_half_mm - 0.5 * hbeam_flange_width_mm - 1.0; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@subsystems/DecayVolume/include/DecayVolume/SBTConfig.h` around lines 129 - 132, Add a brief inline comment in topBottomAvailX() identifying what the 1.0 subtraction represents, such as its clearance or rounding-margin purpose, while leaving the placement calculation unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@subsystems/DecayVolume/include/DecayVolume/SBTConfig.h`:
- Around line 42-46: Update the heliumPieces documentation in
subsystems/DecayVolume/include/DecayVolume/SBTEnvelope.h:85-94 to state that
cfg.helium_clearance_mm defaults to 0.001 mm instead of 0. The SBTConfig.h:42-46
declaration requires no direct change.
- Around line 117-128: Update readSBTConfig validation to require
sensor_clearance_mm to be strictly less than half of container_thickness_mm,
while retaining the existing positive-clearance check. Reject invalid
configurations before the geometry helpers topBottomContainerHalfThickness() and
topBottomSensorInnerY() can use them.
---
Nitpick comments:
In `@subsystems/DecayVolume/include/DecayVolume/SBTConfig.h`:
- Around line 129-132: Add a brief inline comment in topBottomAvailX()
identifying what the 1.0 subtraction represents, such as its clearance or
rounding-margin purpose, while leaving the placement calculation 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: 251daf5c-dbed-4f4f-80f1-e1bffcf6b210
📒 Files selected for processing (9)
subsystems/DecayVolume/include/DecayVolume/DecayVolumeFactory.hsubsystems/DecayVolume/include/DecayVolume/SBTConfig.hsubsystems/DecayVolume/include/DecayVolume/SBTEnvelope.hsubsystems/DecayVolume/sbt.tomlsubsystems/DecayVolume/src/DecayVolumeFactory.cppsubsystems/DecayVolume/src/SBTEnvelope.cppsubsystems/DecayVolume/src/SBTSensorBuilder.cppsubsystems/DecayVolume/src/SBTStructureBuilder.cppsubsystems/DecayVolume/test_decayvolume.cpp
Comment on lines
+42
to
+46
| // Gap left between the helium and the innermost SBT material, along the | ||
| // coordinate axis. Not a safety margin — just enough to avoid coincident | ||
| // surfaces, which Geant4's navigator handles badly. | ||
| // NOTE: this default must track sbt.toml. | ||
| double helium_clearance_mm = 0.001; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify sbt.toml's helium_clearance_mm matches the new C++ default.
fd sbt.toml subsystems/DecayVolume --exec cat {}Repository: ShipSoft/Geometry
Length of output: 3807
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- SBTConfig.h excerpt ---\n'
sed -n '35,55p' subsystems/DecayVolume/include/DecayVolume/SBTConfig.h
printf '\n--- SBTEnvelope.h excerpt ---\n'
sed -n '78,102p' subsystems/DecayVolume/include/DecayVolume/SBTEnvelope.hRepository: ShipSoft/Geometry
Length of output: 2183
Update the heliumPieces doc to match the default. cfg.helium_clearance_mm is 0.001 mm in SBTConfig.h, so SBTEnvelope.h should stop describing it as “0 by default”.
📍 Affects 2 files
subsystems/DecayVolume/include/DecayVolume/SBTConfig.h#L42-L46(this comment)subsystems/DecayVolume/include/DecayVolume/SBTEnvelope.h#L85-L94
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@subsystems/DecayVolume/include/DecayVolume/SBTConfig.h` around lines 42 - 46,
Update the heliumPieces documentation in
subsystems/DecayVolume/include/DecayVolume/SBTEnvelope.h:85-94 to state that
cfg.helium_clearance_mm defaults to 0.001 mm instead of 0. The SBTConfig.h:42-46
declaration requires no direct change.
readSBTConfig required sensor_clearance_mm > 0 but never bounded it from above.
It is trimmed off each side of a top/bottom container, so any value >= half the
container thickness makes SBTConfig::topBottomContainerHalfThickness() negative.
That is worse than a degenerate GeoTrap. topBottomSensorInnerY() subtracts the
half-thickness, so a negative one reports the scintillator's inner face as
further out than it really is, and SBTEnvelope sizes the helium into the
sensors. The config was accepted and the overlap was silent.
Guard it in readSBTConfig, alongside the existing hbeam_height check.
Also correct three stale comments:
- SBTEnvelope.h said helium_clearance_mm defaults to 0; it is 0.001 mm.
- SBTConfig.cpp still described the pre-SBTEnvelope inset formula.
- sbt.toml invited setting helium_clearance_mm = 0.0, which readSBTConfig
rejects. Exact contact is reachable only by passing an SBTConfig directly.
No geometry change; all 15 envelope sweep configurations unaffected.
olantwin
requested changes
Jul 14, 2026
olantwin
left a comment
olantwin
left a comment
Contributor
There was a problem hiding this comment.
The helium_clearance_mm config variable is confusing, grepping through the code I can find places where it's documented as:
- 0 by default (in the code the default actually is 0.001
- required to be positive (i.e. > 0)
- required to be not < 0 (i.e. ≥ 0)
This should be clarified in the comments. One of the checks also seems to be redundant.
Furthermore, the zEnt_mm seems to be dead pretty much anywhere. We might as well remove it, unless the plan is to use it in future.
Contributor
There was a problem hiding this comment.
I think this and some other includes might not be needed anymore here.
jasminwss
reviewed
Jul 14, 2026
|
|
||
| // ── Sensor containers / cells ─────────────────────────────────────── | ||
| double container_thickness_mm = 225.0; | ||
| double wall_thickness_mm = 5.0; |
There was a problem hiding this comment.
as far as I know the aluminum walls are either 6 (Ring 1 to 6) or 8 mm thick.
Contributor
Author
There was a problem hiding this comment.
This is an old version of the geometry but I will fix it here
…code
Review follow-up (olantwin).
helium_clearance_mm was documented three incompatible ways: "0 by default" (it
is 0.001), "must be > 0" (readSBTConfig), and "must be >= 0" (SBTEnvelope). The
real contract is >= 0 — zero is legal and gives exact face-to-face contact
between the helium and the SBT, which the parameter sweep exercises.
Drop the readSBTConfig check rather than the SBTEnvelope one: it is both
redundant and stricter than the contract, and SBTEnvelope has to validate anyway
for callers that construct an SBTConfig directly. State the bound once, in
SBTConfig.h, and name SBTEnvelope as its only consumer.
Also remove code the earlier refactor orphaned:
- the zEnt_mm parameter on the frustum lambdas in SBTStructureBuilder, dead
since they were routed through SBTConfig::xHalfAt/yHalfAt (15 call sites);
- the GeoTrap, GeoNameTag, GeoTransform, GeoDefinitions and <vector> includes
in DecayVolumeFactory, unused since buildHelium moved to SBTEnvelope.
No geometry change; all 15 envelope sweep configurations unaffected.
Review follow-up. minExpectedSeparation corrected only for single-axis tilt (max(gx,gy)). A surface tilted in both x and y tilts by sqrt(gx^2+gy^2), whose true Euclidean gap is below the old bound; at clr=10 that gap is mm-scale and would reject correct geometry if a doubly-tapered face were ever the binding pair. Use sqrt(1+gx^2+gy^2). The SAT axis set is exact only for planar-faced traps. Every trap built today is planar (dxdyn==dxdyp, alpha==0), but nothing enforced it; a future sheared trap would let separation() call an overlapping pair disjoint. Assert planarity in trapVertices so that fails loudly instead. Refresh README: the helium is 20 inscribed GeoTrap slabs (3400 traps, 3712 children), not a single central frustum, and is derived from the SBT surfaces. Document, but do not fix, a known conservative limitation: the Y envelope caps the helium at the longitudinal-beam inner flange over the full sub-frustum, but the beams stand off ~142 mm from each end, so the helium is up to ~13.5 mm short of the scintillator in those bands (~0.18 m^3, 0.04% of fiducial volume). Never overlaps; reclaiming it needs the flange z-projection handled and is deferred. No geometry change; SBTEnvelope.cpp diff is comment-only.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@subsystems/DecayVolume/README.md`:
- Around line 39-41: Correct the geometry-tree entries for structure_* and
sensors_* in the README to reflect the flat hierarchy described by
test_decayvolume.cpp: make both direct children of /SHiP/decay_volume, or
explicitly mark sbt as a naming/grouping convention rather than a physical
volume. Keep the listed geometry types and counts 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: 63a67ea1-846d-4cf8-9246-ecdc6b91d5b1
📒 Files selected for processing (3)
subsystems/DecayVolume/README.mdsubsystems/DecayVolume/src/SBTEnvelope.cppsubsystems/DecayVolume/test_decayvolume.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- subsystems/DecayVolume/src/SBTEnvelope.cpp
- subsystems/DecayVolume/test_decayvolume.cpp
The geometry-tree diagram indented structure_* and sensors_* under sbt, implying a physical grouping volume. There is none: the builders add every piece as a direct child of /SHiP/decay_volume, and /sbt/ is only a prefix on the volume names. Note that the tree is by name, not containment, consistent with the flat hierarchy test_decayvolume asserts. Names, types and counts unchanged.
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
3 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.
The helium decay region was sized by an inset expression in DecayVolumeFactory that duplicated, by hand, the placement rules used by SBTSensorBuilder and SBTStructureBuilder. The two were not linked, and the copy was wrong in two ways:
It assumed the sensors' inner surface was linear in Z. It is not: placeSideContainer gives each container a flat outer face over the first zSplitOffset() of every sub-frustum so as to clear the columns, so the free region is a sawtooth. The helium overlapped the side containers by 122.6 mm.
It assumed the innermost material in Y was scintillator. It is not: the top/bottom longitudinal beams straddle the sensor plane (the web is omitted because it would pass through the cells) and their inner flange hangs below it, into the decay region. The helium overlapped those flanges by 2.5 mm.
Neither was detectable: OverlapCheck is a subsystem-level bounding-box test and does not probe siblings inside decay_volume.
Stop maintaining a second model of the geometry. The placement rules now live on SBTConfig as primitives (sideContainerCentreX, topBottomContainerCentreY, longBeamCentreY, zSplitOffset, xHalfAt/yHalfAt); both builders place with them, and the new SBTEnvelope derives the inner free surface from the same expressions. A change to a placement rule now moves the helium automatically.
The helium becomes a stack of GeoTraps (2 per sub-frustum, 20 total) so it can follow the sawtooth exactly, and is inset from the true inner surface by helium_clearance_mm, now 1 um — enough to avoid coincident boundaries, which Geant4's navigator handles badly, and far below any engineering tolerance.
Verification is now a property, not a formula: SAT (exact for the convex hexahedra involved) between every helium slab and every SBT volume in the built tree, asserting both no protrusion and no margin. A parameter sweep repeats it across 14 SBT configurations, so the guarantee survives re-parameterisation rather than holding at one lucky point.
Fiducial helium volume: 534.80 -> 485.85 m^3. The old figure included steel.
Closes #58
Summary by CodeRabbit