Port upstream Athena improvements from main - #106
Conversation
|
Warning Review limit reached
Next review available in: 28 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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR performs a broad const-correctness and ownership refactor across FastCaloSim Core, Param, and Extrapolation headers/sources: converting value-returning/passing APIs to const-reference and move semantics, replacing typedef structs with named structs, adding default member initializers, switching to std:: math functions, and adding null/zero-division guards. ChangesAPI ownership, initialization, and safety refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
jbeirer
changed the title
jbeirer
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
source/Core/TFCS1DFunctionHistogram.cxx (1)
27-35: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard against zero-total-weight histograms and use RAII for the clone.
h_clone->Scale(1.0 / h_clone->Integral())will produceinf/NaNwhen the histogram integral is zero and poisonhistoVals. Replacing the rawTH1D*/deletepair withstd::unique_ptr<TH1D>would also make this exception-safe.🤖 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 `@source/Core/TFCS1DFunctionHistogram.cxx` around lines 27 - 35, The histogram normalization in TFCS1DFunctionHistogram is unsafe when the source integral is zero and it also manages the cloned TH1D manually. Update the cloning and normalization logic around the hist/h_clone block to compute the integral first, skip or handle the zero-total-weight case before calling Scale, and replace the raw TH1D* plus delete with a std::unique_ptr<TH1D> so the clone is RAII-managed and exception-safe. Keep the existing histoVals population logic in place after the normalization guard.source/Core/TFCSGANXMLParameters.cxx (1)
48-142: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse RAII for
xmlDocPtrto prevent leak on exception.
xmlFreeDoc(doc)at line 142 is unreachable ifstd::stoi(line 34) orstd::stod(line 95) throws during parsing, leaking the XML document. This is now reachable because the PR switched fromatoi/atof(which never throw) tostd::stoi/std::stod(which throw on malformed input).Proposed fix using unique_ptr with custom deleter
xmlDocPtr doc = xmlParseFile(xmlFullFileName.c_str()); if (!doc) { FCS_MSG_WARNING("Failed to parse XML file: " << xmlFullFileName); return; } + auto docGuard = std::unique_ptr<xmlDoc, decltype(&xmlFreeDoc)>( + doc, &xmlFreeDoc);Then remove the manual
xmlFreeDoc(doc)at line 142; theunique_ptrdestructor handles it.🤖 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 `@source/Core/TFCSGANXMLParameters.cxx` around lines 48 - 142, The XML document in TFCSGANXMLParameters.cxx is manually freed with xmlFreeDoc, but that cleanup is skipped if parsing of attributes or edge values throws from the numeric conversions used in the XML traversal. Wrap the xmlParseFile result in an RAII owner with a custom deleter in TFCSGANXMLParameters::ReadXML (or the equivalent parsing routine), so the document is always released even when std::stoi/std::stod fail, and remove the explicit xmlFreeDoc call at the end.
🧹 Nitpick comments (4)
include/FastCaloSim/Core/TFCS1DFunctionHistogram.h (1)
36-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConst-qualify these read-only accessors.
get_HistoBorders()/get_HistoContents()only return const references to members but are declared as non-const member functions, so they can't be invoked on aconst TFCS1DFunctionHistogram&. Marking themconstmakes the accessors usable from const contexts without changing behavior.♻️ Proposed change
- const std::vector<float>& get_HistoBorders() { return m_HistoBorders; }; - const std::vector<float>& get_HistoContents() { return m_HistoContents; }; + const std::vector<float>& get_HistoBorders() const { return m_HistoBorders; }; + const std::vector<float>& get_HistoContents() const { return m_HistoContents; };🤖 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 `@include/FastCaloSim/Core/TFCS1DFunctionHistogram.h` around lines 36 - 37, The read-only accessors in TFCS1DFunctionHistogram are missing const qualification, so they cannot be called on const instances. Update get_HistoBorders() and get_HistoContents() to be const member functions while keeping their return types as const references to m_HistoBorders and m_HistoContents, so the class remains unchanged in behavior but usable from const contexts.include/FastCaloSim/Core/TFCSHitCellMappingWiggle.h (1)
42-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMark the reference-returning accessors
const.
get_functions()andget_bin_low_edges()returnconstreferences but are non-constmember functions, unlikeget_function(int)andget_bin_low_edge(int)right above them. This blocks calling them through aconst TFCSHitCellMappingWiggle&. Addconstfor consistency and to keep these usable from const contexts.♻️ Proposed fix
- const std::vector<const TFCS1DFunction*>& get_functions() - { - return m_functions; - }; - const std::vector<float>& get_bin_low_edges() { return m_bin_low_edge; }; + const std::vector<const TFCS1DFunction*>& get_functions() const + { + return m_functions; + }; + const std::vector<float>& get_bin_low_edges() const { return m_bin_low_edge; };🤖 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 `@include/FastCaloSim/Core/TFCSHitCellMappingWiggle.h` around lines 42 - 46, The reference-returning accessors in TFCSHitCellMappingWiggle are missing const qualification, which prevents using them on const instances. Update get_functions() and get_bin_low_edges() in TFCSHitCellMappingWiggle to be const member functions, matching the style of get_function(int) and get_bin_low_edge(int), so callers can access these references from const contexts.source/Core/TFCSGANEtaSlice.cxx (1)
108-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
std::movefor consistency with othercreate()calls.Lines 92, 100, and 115 all use
std::move(inputFileName), but line 108 passesinputFileNameby copy. The string is reassigned at line 112, so moving is safe.Proposed fix
- m_net_high = TFCSNetworkFactory::create(inputFileName); + m_net_high = TFCSNetworkFactory::create(std::move(inputFileName));🤖 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 `@source/Core/TFCSGANEtaSlice.cxx` at line 108, The create() call in TFCSGANEtaSlice::createNetworkData uses inputFileName inconsistently by passing it by copy instead of move. Update the m_net_high assignment to use std::move(inputFileName), matching the other TFCSNetworkFactory::create calls in this method and keeping ownership transfer consistent before the variable is reassigned later.source/Core/TFCSEnergyAndHitGANV2.cxx (1)
142-145: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftConsider narrowing the mutex lock scope for multi-threaded performance.
The
std::scoped_lockat line 144 is held through the entirefillEnergyfunction (lines 144–527), serializing not just GAN inference but all hit creation, random number generation, and chain simulation. The comment states the goal is to "protect the GAN inference and network outputs," butoutputsis already a local copy (line 145). Ifm_sliceandm_paramare stable during simulation (i.e.,LoadGANis only called during initialization), the lock could potentially be released afterGetNetworkOutputsandGetFitResultsare retrieved, allowing concurrent hit simulation with separatesimulstate/truthper thread.This requires verifying that chain elements accessed via
simulate_hit(lines 259, 481) are either stateless or independently thread-safe. If they have mutable per-call state, the broad lock is necessary.🤖 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 `@source/Core/TFCSEnergyAndHitGANV2.cxx` around lines 142 - 145, The mutex in fillEnergy is broader than needed and currently serializes the whole simulation path. Keep the lock only around the shared GAN access in TFCSEnergyAndHitGANV2::fillEnergy, specifically the GetNetworkOutputs/GetFitResults retrieval from m_slice/m_param, and release it before hit creation, random generation, and chain simulation if those operations only use per-call state. Verify any simulate_hit chain elements are thread-safe or stateless before narrowing the scope.
🤖 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 `@include/FastCaloSim/Core/TFCSBinnedShower.h`:
- Around line 128-135: `TFCSBinnedShower::get_eventlibrary()` and
`TFCSBinnedShower::get_coordinates()` are missing `const` on the member function
even though they return const references. Update both getters in
`TFCSBinnedShower` to be const-qualified, matching the existing const-correct
accessors like `get_hdf5_path()`, `get_sub_bin_distribution()`, and
`get_upscaling_energies()`, so they can be called on const instances.
In `@include/FastCaloSim/Core/TFCSBinnedShowerONNX.h`:
- Line 78: Add a const qualifier to the TFCSBinnedShowerONNX::get_coordinates()
accessor so it matches the other read-only getters and the existing const
overloads. Update the declaration in TFCSBinnedShowerONNX to make the method
const while keeping the return type as a const reference, and ensure any callers
still compile with the new const-correct signature.
In `@include/FastCaloSim/Core/TFCSGANEtaSlice.h`:
- Around line 55-58: The `GetExtrapolatorWeights()` accessor in
`TFCSGANEtaSlice` is missing a `const` qualifier, unlike `GetFitResults()`, so
update the method signature to make it callable on a `const TFCSGANEtaSlice&`
and keep the class consistent with the PR’s const-correctness. Locate the
non-const `GetExtrapolatorWeights()` method and change only its
declaration/definition so it remains a read-only accessor returning
`m_extrapolatorWeights`.
In `@source/Core/TFCSGANEtaSlice.cxx`:
- Around line 139-140: The histogram lookup in TFCSGANEtaSlice::GetHisto uses
file->Get(histoName.c_str()) without checking for a null result before calling
h1->Integral(), which can dereference a missing histogram. Add a null check
immediately after the Get call in TFCSGANEtaSlice::GetHisto, and if h1 is
nullptr, throw a std::runtime_error consistent with the existing bad-file
handling before any Integral() access.
In `@source/Core/TFCSGANXMLParameters.cxx`:
- Around line 94-96: Update the r_edges parsing loop in TFCSGANXMLParameters so
it does not pass empty or malformed tokens directly into std::stod. In the
tokenization logic that fills edges, add handling to skip empty tokens
(including cases from trailing or repeated commas) before conversion, or
otherwise guard the std::stod call with error handling so malformed input does
not throw. Keep the fix localized to the parsing block that uses std::getline
and edges.push_back.
---
Outside diff comments:
In `@source/Core/TFCS1DFunctionHistogram.cxx`:
- Around line 27-35: The histogram normalization in TFCS1DFunctionHistogram is
unsafe when the source integral is zero and it also manages the cloned TH1D
manually. Update the cloning and normalization logic around the hist/h_clone
block to compute the integral first, skip or handle the zero-total-weight case
before calling Scale, and replace the raw TH1D* plus delete with a
std::unique_ptr<TH1D> so the clone is RAII-managed and exception-safe. Keep the
existing histoVals population logic in place after the normalization guard.
In `@source/Core/TFCSGANXMLParameters.cxx`:
- Around line 48-142: The XML document in TFCSGANXMLParameters.cxx is manually
freed with xmlFreeDoc, but that cleanup is skipped if parsing of attributes or
edge values throws from the numeric conversions used in the XML traversal. Wrap
the xmlParseFile result in an RAII owner with a custom deleter in
TFCSGANXMLParameters::ReadXML (or the equivalent parsing routine), so the
document is always released even when std::stoi/std::stod fail, and remove the
explicit xmlFreeDoc call at the end.
---
Nitpick comments:
In `@include/FastCaloSim/Core/TFCS1DFunctionHistogram.h`:
- Around line 36-37: The read-only accessors in TFCS1DFunctionHistogram are
missing const qualification, so they cannot be called on const instances. Update
get_HistoBorders() and get_HistoContents() to be const member functions while
keeping their return types as const references to m_HistoBorders and
m_HistoContents, so the class remains unchanged in behavior but usable from
const contexts.
In `@include/FastCaloSim/Core/TFCSHitCellMappingWiggle.h`:
- Around line 42-46: The reference-returning accessors in
TFCSHitCellMappingWiggle are missing const qualification, which prevents using
them on const instances. Update get_functions() and get_bin_low_edges() in
TFCSHitCellMappingWiggle to be const member functions, matching the style of
get_function(int) and get_bin_low_edge(int), so callers can access these
references from const contexts.
In `@source/Core/TFCSEnergyAndHitGANV2.cxx`:
- Around line 142-145: The mutex in fillEnergy is broader than needed and
currently serializes the whole simulation path. Keep the lock only around the
shared GAN access in TFCSEnergyAndHitGANV2::fillEnergy, specifically the
GetNetworkOutputs/GetFitResults retrieval from m_slice/m_param, and release it
before hit creation, random generation, and chain simulation if those operations
only use per-call state. Verify any simulate_hit chain elements are thread-safe
or stateless before narrowing the scope.
In `@source/Core/TFCSGANEtaSlice.cxx`:
- Line 108: The create() call in TFCSGANEtaSlice::createNetworkData uses
inputFileName inconsistently by passing it by copy instead of move. Update the
m_net_high assignment to use std::move(inputFileName), matching the other
TFCSNetworkFactory::create calls in this method and keeping ownership transfer
consistent before the variable is reassigned later.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 34e4db32-4b0d-4d80-8cda-726899008e28
📒 Files selected for processing (39)
include/FastCaloSim/Core/TFCS1DFunctionHistogram.hinclude/FastCaloSim/Core/TFCS1DFunctionRegressionTF.hinclude/FastCaloSim/Core/TFCSBinnedShower.hinclude/FastCaloSim/Core/TFCSBinnedShowerONNX.hinclude/FastCaloSim/Core/TFCSEnergyAndHitGANV2.hinclude/FastCaloSim/Core/TFCSFlatLateralShapeParametrization.hinclude/FastCaloSim/Core/TFCSGANEtaSlice.hinclude/FastCaloSim/Core/TFCSGANXMLParameters.hinclude/FastCaloSim/Core/TFCSHitCellMappingWiggle.hinclude/FastCaloSim/Core/TFCSLateralShapeParametrizationFluctChain.hinclude/FastCaloSim/Core/TFCSLateralShapeParametrizationHitBase.hinclude/FastCaloSim/Core/TFCSMLCalorimeterSimulator.hinclude/FastCaloSim/Core/TFCSPhiModulationCorrection.hinclude/FastCaloSim/Core/TFCSSimulationState.hinclude/FastCaloSim/Core/TFCSVoxelHistoLateralCovarianceFluctuations.hinclude/FastCaloSim/Extrapolation/FastCaloSimCaloExtrapolation.hparam/include/FastCaloSimParam/FCS_Cell.hsource/Core/TFCS1DFunction.cxxsource/Core/TFCS1DFunctionHistogram.cxxsource/Core/TFCS1DFunctionRegression.cxxsource/Core/TFCS2DFunction.cxxsource/Core/TFCSBinnedShower.cxxsource/Core/TFCSBinnedShowerBase.cxxsource/Core/TFCSBinnedShowerONNX.cxxsource/Core/TFCSEnergyAndHitGANV2.cxxsource/Core/TFCSEnergyBinParametrization.cxxsource/Core/TFCSGANEtaSlice.cxxsource/Core/TFCSGANLWTNNHandler.cxxsource/Core/TFCSGANXMLParameters.cxxsource/Core/TFCSHitCellMappingWiggle.cxxsource/Core/TFCSMLCalorimeterSimulator.cxxsource/Core/TFCSNetworkFactory.cxxsource/Core/TFCSONNXHandler.cxxsource/Core/TFCSParametrizationBase.cxxsource/Core/TFCSParametrizationChain.cxxsource/Core/TFCSPhiModulationCorrection.cxxsource/Core/TFCSSimpleLWTNNHandler.cxxsource/Core/TFCSVoxelHistoLateralCovarianceFluctuations.cxxsource/Core/VNetworkLWTNN.cxx
1 participant
Ports upstream Athena changes in preparation for full migration:
with a mutex (ATLASSIM-7031 thread-safety fixes)
re-initialization, TFile handling in TFCSGANEtaSlice/TFCSBinnedShowerONNX
FCS_Cell structs, CylinderIntersections
TFCSEnergyBinParametrization
C-style casts, const-ref getters/parameters, std::move on push_backs
Summary by CodeRabbit
Bug Fixes
Refactor