added SST#19
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)
📝 WalkthroughWalkthroughAdds complete straw tracking station geometry: four stations with four stereo views per station, staggered straw sub-layers, and hollow frames. Introduces Mylar and ArCO2 materials; refactors TrackersFactory into hierarchical builder methods; adds validation tests and documentation updates. ChangesStraw Tracker Geometry and Materials
🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
🤖 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/Trackers/README.md`:
- Around line 21-33: Add the language tag "text" to the fenced code block that
contains the geometry tree starting with "/SHiP/trackers (Air, 3000 × 3430 ×
6000 mm half-extents)" by changing the opening triple backticks to ```text so
the block is recognized by markdownlint (MD040) while keeping the content
unchanged.
In `@subsystems/Trackers/src/TrackersFactory.cpp`:
- Around line 127-129: Replace the non-portable M_PI usage in the angleRad
calculation with the C++20 constant: include <numbers> and use
std::numbers::pi_v<double> in place of M_PI in the expression computing angleRad
(the line computing const double angleRad = stereoSignedDeg(v) * M_PI / 180.0;)
and ensure the same replacement is applied where GeoTrf::RotateZ3D(angleRad) is
used so the code builds with CMAKE_CXX_EXTENSIONS=OFF.
🪄 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: b18c53ba-40bb-4e7a-84e3-0817c2db0df1
📒 Files selected for processing (6)
README.mdsrc/SHiPMaterials.cppsubsystems/Trackers/README.mdsubsystems/Trackers/include/Trackers/TrackersFactory.hsubsystems/Trackers/src/TrackersFactory.cppsubsystems/Trackers/test_trackers.cpp
| ``` | ||
| TrackersContainer (Air, 6000×6860×6000 mm) | ||
| ├─ TrackerStation_1 (Air, 6000×6860×1000 mm) z = 84070 mm | ||
| ├─ TrackerStation_2 (Air) z = 86070 mm | ||
| ├─ TrackerStation_3 (Air) z = 93070 mm | ||
| └─ TrackerStation_4 (Air) z = 95070 mm | ||
| /SHiP/trackers (Air, 3000 × 3430 × 6000 mm half-extents) | ||
| ├─ /SHiP/trackers/station_<n> (Air, 3000 × 3430 × 500 mm) n = 1..4 | ||
| │ └─ /SHiP/trackers/station_<n>/view_<v>/envelope (Air) v = 0..3 | ||
| │ (rotated about Z by the view stereo angle) | ||
| │ ├─ .../frame_body (Aluminium, hollow rectangle = outer − aperture) | ||
| │ ├─ .../sublayer_0_body (Air slab, 300 straws) | ||
| │ └─ .../sublayer_1_body (Air slab, 300 straws, half-pitch staggered) | ||
| │ └─ .../straw_<i> | ||
| │ └─ straw_wall_<uid> (Mylar tube) | ||
| │ └─ straw_gas_<uid> (ArCO2_70_30 tube) | ||
| └─ /SHiP/trackers/tracker_magnet (Air, inert marker box) | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to the fenced code block to satisfy markdownlint.
The geometry tree fence is missing a language identifier (MD040). Adding text keeps the current rendering and clears lint noise.
Proposed patch
-```
+```text
/SHiP/trackers (Air, 3000 × 3430 × 6000 mm half-extents)
├─ /SHiP/trackers/station_<n> (Air, 3000 × 3430 × 500 mm) n = 1..4
│ └─ /SHiP/trackers/station_<n>/view_<v>/envelope (Air) v = 0..3
@@
-```
+```🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 21-21: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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/Trackers/README.md` around lines 21 - 33, Add the language tag
"text" to the fenced code block that contains the geometry tree starting with
"/SHiP/trackers (Air, 3000 × 3430 × 6000 mm half-extents)" by changing the
opening triple backticks to ```text so the block is recognized by markdownlint
(MD040) while keeping the content unchanged.
| const double angleRad = stereoSignedDeg(v) * M_PI / 180.0; | ||
| const GeoTrf::Transform3D viewTrf = | ||
| GeoTrf::Translate3D(0.0, 0.0, zView) * GeoTrf::RotateZ3D(angleRad); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== M_PI usages =="
rg -n '\bM_PI\b'
echo
echo "== C++ standard / extension settings in build files =="
rg -n 'CMAKE_CXX_STANDARD|CMAKE_CXX_EXTENSIONS|std=c\+\+|std=gnu\+\+'Repository: ShipSoft/Geometry
Length of output: 528
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="subsystems/Trackers/src/TrackersFactory.cpp"
echo "== File: $FILE =="
echo
echo "== Header includes (top 80 lines) =="
nl -ba "$FILE" | head -n 80
echo
echo "== Context around line 127 =="
nl -ba "$FILE" | sed -n '110,145p'
echo
echo "== Context around line 240 =="
nl -ba "$FILE" | sed -n '220,255p'
echo
echo "== Any existing std::numbers::pi_v or numbers header =="
rg -n 'std::numbers::pi_v|<numbers>' "$FILE"Repository: ShipSoft/Geometry
Length of output: 216
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="subsystems/Trackers/src/TrackersFactory.cpp"
echo "== File: $FILE =="
echo
echo "== Header includes (top 90 lines) =="
cat -n "$FILE" | head -n 90
echo
echo "== Context around line 127 =="
cat -n "$FILE" | sed -n '105,150p'
echo
echo "== Context around line 240 =="
cat -n "$FILE" | sed -n '215,255p'
echo
echo "== Any existing std::numbers::pi_v or numbers header (in file) =="
rg -n 'std::numbers::pi_v|<numbers>' "$FILE" || trueRepository: ShipSoft/Geometry
Length of output: 9107
Replace non-standard M_PI usage with std::numbers::pi_v<double> (C++20 portability)
subsystems/Trackers/src/TrackersFactory.cpp uses M_PI at the flagged sites; with CMAKE_CXX_STANDARD=20 and CMAKE_CXX_EXTENSIONS=OFF, relying on M_PI is non-portable. Switch to std::numbers::pi_v<double> and add <numbers>.
Proposed patch
`#include` <cmath>
+#include <numbers>
`#include` <string>
@@
- const double angleRad = stereoSignedDeg(v) * M_PI / 180.0;
+ const double angleRad = stereoSignedDeg(v) * std::numbers::pi_v<double> / 180.0;
@@
- GeoTrf::Translate3D(0.0, yStraw, 0.0) * GeoTrf::RotateY3D(M_PI / 2.0);
+ GeoTrf::Translate3D(0.0, yStraw, 0.0) *
+ GeoTrf::RotateY3D(std::numbers::pi_v<double> / 2.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/Trackers/src/TrackersFactory.cpp` around lines 127 - 129, Replace
the non-portable M_PI usage in the angleRad calculation with the C++20 constant:
include <numbers> and use std::numbers::pi_v<double> in place of M_PI in the
expression computing angleRad (the line computing const double angleRad =
stereoSignedDeg(v) * M_PI / 180.0;) and ensure the same replacement is applied
where GeoTrf::RotateZ3D(angleRad) is used so the code builds with
CMAKE_CXX_EXTENSIONS=OFF.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Things seem to be in order. The SST is not nearly as detailed as the calo (because I don't know as much about that detector) but it should be functional. There is no frame because I don't know what it will look like. I suggest adding a .toml configuration in a separate commit, preferably with more input from the subsystem responsibles. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
2 participants
SST was added, no supporting structure but the B-field container is present and should support the map txt files as input
Summary by CodeRabbit
New Features
Materials
Documentation
Tests