Add binned shower code#84
Open
jbeirer wants to merge 29 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds a binned-shower simulation framework: a new abstract base and two implementations (HDF5-driven template and ONNX-driven ML), an ML simulator wrapper, phi-modulation and voxel-covariance fluctuation modules, hit-chain/hit-index extensions, ROOT/HDF5 I/O, and CMake HDF5 integration. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Simulation Client
participant Binned as TFCSBinnedShower / TFCSBinnedShowerONNX
participant ML as TFCSMLCalorimeterSimulator
participant HDF5 as HDF5 / ROOT files
participant SimState as TFCSSimulationState
Client->>Binned: simulate(simulstate, truth, extrapol)
activate Binned
Binned->>HDF5: (optional) load metadata / event library
HDF5-->>Binned: layer bin boundaries, event meta
alt ONNX flow
Binned->>ML: predictVoxels(simulstate, eta, energy)
activate ML
ML-->>Binned: voxel outputs / event_t
deactivate ML
end
Binned->>Binned: select event (match/cherry-pick/eta/uniform)
Binned->>SimState: store event aux-info (BSNHits/BSELayer), compute per-layer energies & n_hits
loop per requested hit
Client->>Binned: simulate_hit(hit, simulstate,...)
Binned->>Binned: get_hit_position_and_energy(layer, hit_index)
alt upscaling enabled (first use)
Binned->>HDF5: load sub-bin distributions / roots matrices
HDF5-->>Binned: sub-bin matrices & energies
Binned->>Binned: upscale coordinates/energy
end
Binned->>SimState: set hit position & energy
end
Binned->>Binned: delete_event() cleanup
deactivate Binned
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/FastCaloSim/Core/TFCSLateralShapeParametrizationHitBase.h (1)
132-145:⚠️ Potential issue | 🔴 CriticalInitialize and populate
m_hit_indexbefore exposing it.
idx()adds readable state, butHitnever initializes it,reset()never clears it, and this type provides no way for callers to set it. Any consumer ofidx()will read an indeterminate value.Proposed fix
Hit() : m_eta_x(0.) , m_phi_y(0.) , m_z(0.) , m_E(0.) , m_useXYZ(false) , m_center_r(0.) , m_center_z(0.) , m_center_eta(0.) - , m_center_phi(0.) {}; + , m_center_phi(0.) + , m_hit_index(0) {}; Hit(float eta, float phi, float E) : m_eta_x(eta) , m_phi_y(phi) , m_E(E) , m_useXYZ(false) , m_center_r(0.) , m_center_z(0.) , m_center_eta(0.) - , m_center_phi(0.) {}; + , m_center_phi(0.) + , m_hit_index(0) {}; Hit(float x, float y, float z, float E) : m_eta_x(x) , m_phi_y(y) , m_z(z) , m_E(E) , m_useXYZ(true) , m_center_r(0.) , m_center_z(0.) , m_center_eta(0.) - , m_center_phi(0.) {}; + , m_center_phi(0.) + , m_hit_index(0) {}; @@ inline void reset() { m_eta_x = 0.; m_phi_y = 0.; m_z = 0.; m_E = 0.; m_useXYZ = false; + m_hit_index = 0; } @@ - inline const auto& idx() { return m_hit_index; }; + inline long unsigned int idx() const { return m_hit_index; } + inline void set_idx(long unsigned int hit_index) { m_hit_index = hit_index; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/FastCaloSim/Core/TFCSLateralShapeParametrizationHitBase.h` around lines 132 - 145, m_hit_index is never initialized or cleared, so callers of idx() can read garbage; initialize m_hit_index in the TFCSLateralShapeParametrizationHitBase constructor (set to a sentinel like ~0u or 0), ensure reset() clears it back to the sentinel, and expose a controlled mutator (e.g., set_idx(unsigned long) or assign it where hits are created) so code using idx() always reads a well-defined value; update uses in methods that construct/populate hits to call the new setter or assign m_hit_index during construction/initialization (referencing m_hit_index, idx(), constructor of TFCSLateralShapeParametrizationHitBase, and reset()).
🧹 Nitpick comments (15)
include/FastCaloSim/Core/TFCSBinnedShowerONNX.h (2)
143-143: Fix typo: "mamimum" → "maximum".🧹 Proposed fix
- // What is the mamimum number of hits per voxel (for runtime reasons) + // What is the maximum number of hits per voxel (for runtime reasons)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/FastCaloSim/Core/TFCSBinnedShowerONNX.h` at line 143, Comment typo in TFCSBinnedShowerONNX.h: change the spelling "mamimum" to "maximum" in the comment that reads "What is the mamimum number of hits per voxel (for runtime reasons)"; update the comment text where this appears (e.g., near the declaration/usage of TFCSBinnedShowerONNX-related member or constant) so it reads "What is the maximum number of hits per voxel (for runtime reasons)".
53-58: TODO comment is resolved - destructor handles deletion.The TODO comment asks about deletion of
m_ai_simulator. Looking atTFCSBinnedShowerONNX.cxxlines 46-53, the destructor properly deletes and nulls this pointer. The TODO can be removed.🧹 Proposed cleanup
void load_simulator(const std::string& filename) { - // TODO: Do I have to delete it again? m_ai_simulator = new TFCSMLCalorimeterSimulator(); m_ai_simulator->loadSimulator(filename); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/FastCaloSim/Core/TFCSBinnedShowerONNX.h` around lines 53 - 58, Remove the outdated TODO in TFCSBinnedShowerONNX::load_simulator that asks about deleting m_ai_simulator; keep the existing allocation code (m_ai_simulator = new TFCSMLCalorimeterSimulator(); m_ai_simulator->loadSimulator(filename);) as the destructor in TFCSBinnedShowerONNX (which deletes and nulls m_ai_simulator) already handles cleanup, so simply delete the comment line and leave the method implementation unchanged.include/FastCaloSim/Core/TFCSBinnedShowerBase.h (2)
11-11: Remove unused<fstream>include.The
<fstream>header is included but not used in this header file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/FastCaloSim/Core/TFCSBinnedShowerBase.h` at line 11, The header TFCSBinnedShowerBase.h currently includes <fstream> which is unused; remove the unused include directive from the top of TFCSBinnedShowerBase.h (the line "#include <fstream>") to clean up includes and avoid unnecessary dependencies, then run a quick rebuild to ensure no compilation errors.
103-106: Remove unused memberm_n_layers.The constant
m_n_layersis declared but never referenced in this header or in the implementation fileTFCSBinnedShowerBase.cxx. SinceCaloCell_ID_FCS::CaloSample_FCS::MaxSamplecan be used directly where needed, this member appears to be dead code.🧹 Proposed cleanup
private: - const long unsigned int m_n_layers = - CaloCell_ID_FCS::CaloSample_FCS::MaxSample; ClassDefOverride(TFCSBinnedShowerBase, 1) // TFCSBinnedShowerBase };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/FastCaloSim/Core/TFCSBinnedShowerBase.h` around lines 103 - 106, Remove the unused private member m_n_layers from class TFCSBinnedShowerBase: delete the declaration "const long unsigned int m_n_layers = CaloCell_ID_FCS::CaloSample_FCS::MaxSample;" from TFCSBinnedShowerBase.h and remove any references/usages in TFCSBinnedShowerBase.cxx (if present), using CaloCell_ID_FCS::CaloSample_FCS::MaxSample directly where needed; leave ClassDefOverride(TFCSBinnedShowerBase, 1) intact and ensure headers/compilation still succeed after the cleanup.source/Core/TFCSBinnedShowerBase.cxx (2)
76-79: Simplify energy setting -set_Efollowed byadd_Eis redundant.Lines 77-78 call
set_E(layer_index, 0)immediately followed byadd_E(layer_index, layer_energy). This can be simplified to a singleset_E(layer_index, layer_energy)call since the reset to zero is immediately overwritten.♻️ Proposed simplification
float layer_energy = get_layer_energy(simulstate, layer_index); - // Reset and set the layer energy - simulstate.set_E(layer_index, 0); - simulstate.add_E(layer_index, layer_energy); + simulstate.set_E(layer_index, layer_energy); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/Core/TFCSBinnedShowerBase.cxx` around lines 76 - 79, Replace the redundant reset+add on the simulstate energy by using a single assignment: remove the simulstate.set_E(layer_index, 0) followed by simulstate.add_E(layer_index, layer_energy) and instead call simulstate.set_E(layer_index, layer_energy) so the energy for the given layer_index is set directly to layer_energy; update references to simulstate.set_E and simulstate.add_E in this block accordingly.
9-12: Remove unused includes in the base class.Several includes appear unused in this base class implementation:
H5Cpp.h,TFile.h,TH2.h,TBuffer.h, andTClass.h. These are likely needed only in derived classes. Removing them reduces compilation time and clarifies dependencies.🧹 Proposed cleanup
`#include` <CLHEP/Random/RanluxEngine.h> -#include <H5Cpp.h> -#include <TFile.h> -#include <TH2.h> `#include` "CLHEP/Random/RandFlat.h" `#include` "FastCaloSim/Core/ICaloGeometry.h" `#include` "FastCaloSim/Core/TFCSExtrapolationState.h" `#include` "FastCaloSim/Core/TFCSLateralShapeParametrizationHitBase.h" `#include` "FastCaloSim/Core/TFCSSimulationState.h" `#include` "FastCaloSim/Core/TFCSTruthState.h" `#include` "HepPDT/ParticleData.hh" -#include "TBuffer.h" -#include "TClass.h"Also applies to: 21-22
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/Core/TFCSBinnedShowerBase.cxx` around lines 9 - 12, In TFCSBinnedShowerBase.cxx (TFCSBinnedShowerBase implementation) remove unused header includes H5Cpp.h, TFile.h, TH2.h, TBuffer.h and TClass.h from the top of the file, leaving only the headers actually used (e.g. CLHEP/Random/RanluxEngine.h and any TFCS* headers referenced by functions in the TFCSBinnedShowerBase class); ensure compilation still passes and add any removed headers back only in derived classes that require HDF5, ROOT TFile/TH2 or ROOT streaming support.source/Core/TFCSPhiModulationCorrection.cxx (1)
139-139: UseATH_MSG_DEBUGinstead ofstd::cout.Line 139 uses
std::coutfor debug output. For consistency with the rest of the codebase, use the ATLAS logging macros.🧹 Proposed fix
if (param) { energy_shifts.at(eta_index) = param->GetVal(); - std::cout << "Energy shift: " << energy_shifts.at(eta_index) << std::endl; + ATH_MSG_DEBUG("Energy shift: " << energy_shifts.at(eta_index)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/Core/TFCSPhiModulationCorrection.cxx` at line 139, Replace the direct std::cout debug print with the ATLAS logging macro: use ATH_MSG_DEBUG to log the energy shift instead of std::cout in TFCSPhiModulationCorrection (the location that prints energy_shifts.at(eta_index)); format the message similarly (e.g. a descriptive string plus the energy_shifts.at(eta_index) value) so it integrates with the component's logger and follows the usual ATH_MSG_* conventions.include/FastCaloSim/Core/TFCSMLCalorimeterSimulator.h (2)
24-33: Consider using C++ struct syntax instead of C-style typedef.The
typedef struct { ... } name;pattern is a C idiom. In C++, you can simply usestruct name { ... };directly, which is more idiomatic.♻️ Proposed simplification
- typedef struct - { + struct layer_t + { std::vector<unsigned int> bin_index_vector; std::vector<float> E_vector; - } layer_t; + }; - typedef struct - { + struct event_t + { std::vector<layer_t> event_data; - } event_t; + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/FastCaloSim/Core/TFCSMLCalorimeterSimulator.h` around lines 24 - 33, Replace the C-style typedef structs with idiomatic C++ struct declarations: change the `typedef struct { std::vector<unsigned int> bin_index_vector; std::vector<float> E_vector; } layer_t;` to a plain `struct layer_t { ... };` and likewise change `typedef struct { std::vector<layer_t> event_data; } event_t;` to `struct event_t { ... };`. Update any forward references or includes if needed, but keep the same type names (`layer_t`, `event_t`) and member names (`bin_index_vector`, `E_vector`, `event_data`) so usage in functions and classes (e.g., anywhere referencing layer_t or event_t) remains unchanged.
72-72: Fix include guard end comment.The closing comment
ISF_TFCSMLCALORIMETERSIMULATOR_Hdoesn't match the actual include guardTFCSMLCALORIMETERSIMULATOR_Hdefined on line 4.🧹 Proposed fix
-#endif //> !ISF_TFCSMLCALORIMETERSIMULATOR_H +#endif //> !TFCSMLCALORIMETERSIMULATOR_H🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/FastCaloSim/Core/TFCSMLCalorimeterSimulator.h` at line 72, The include guard closing comment in TFCSMLCalorimeterSimulator.h is incorrect; update the end-of-file comment to match the actual guard name TFCSMLCALORIMETERSIMULATOR_H (replace ISF_TFCSMLCALORIMETERSIMULATOR_H with TFCSMLCALORIMETERSIMULATOR_H) so the `#endif` comment corresponds to the `#ifndef/`#define at the top of TFCSMLCalorimeterSimulator.h.include/FastCaloSim/Core/TFCSPhiModulationCorrection.h (1)
42-54: Consider returning const references from getters to avoid copies.The getter methods
get_phi_modulation(),get_min_eta(),get_energy_shift(), andget_min_phi()return nested vectors by value, which involves copying potentially large data structures. If these are called frequently, returningconst&would be more efficient.♻️ Proposed optimization
- std::vector<std::vector<std::vector<float>>> get_phi_modulation() const + const std::vector<std::vector<std::vector<float>>>& get_phi_modulation() const { return m_modulation; }; - std::vector<std::vector<float>> get_min_eta() const { return m_min_eta; }; + const std::vector<std::vector<float>>& get_min_eta() const { return m_min_eta; }; // ... similar for other getters🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/FastCaloSim/Core/TFCSPhiModulationCorrection.h` around lines 42 - 54, The getters get_phi_modulation(), get_min_eta(), get_energy_shift(), and get_min_phi() currently return large nested std::vector by value causing unnecessary copies; change their signatures in TFCSPhiModulationCorrection to return const references (e.g. const std::vector<std::vector<std::vector<float>>>& get_phi_modulation() const) and similarly for get_min_eta(), get_energy_shift(), and get_min_phi(), ensuring the returned types match the member types (m_modulation, m_min_eta, m_energy_shift, m_min_phi) and maintain const-correctness so callers cannot modify internal state; update any call sites if they relied on receiving a copy.source/Core/TFCSBinnedShowerONNX.cxx (2)
514-516: Consider whether missing aux data indelete_eventwarrants an error.The
ATH_MSG_ERRORcalls when aux data is not found might be too aggressive. Ifdelete_eventcan be called in scenarios where initialization failed or was skipped, these would generate spurious errors. Consider usingATH_MSG_DEBUGorATH_MSG_WARNINGinstead, or simply omitting the log when data is already null.Also applies to: 523-525, 532-533
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/Core/TFCSBinnedShowerONNX.cxx` around lines 514 - 516, In TFCSBinnedShowerONNX::delete_event, replace the ATH_MSG_ERROR calls that report missing aux data (the three occurrences flagged) with less aggressive logging or no log: either use ATH_MSG_DEBUG (preferred if missing aux data can be normal) or ATH_MSG_WARNING (if it’s unexpected but non-fatal), or simply omit the message when the aux pointer is already null; update the three error sites inside delete_event to follow this change so spurious errors are not emitted when initialization was skipped or failed.
543-546: Use consistent logging instead ofstd::cerrandstd::cout.The error on line 544 uses
std::cerrwhile the rest of the codebase usesATH_MSG_ERROR. Similarly, lines 597-605 usestd::coutfor debug output instead ofATH_MSG_DEBUG. Using consistent logging improves debuggability and allows log level control.♻️ Proposed fix
TFile* file = TFile::Open(filename.c_str(), "READ"); if (!file || file->IsZombie()) { - std::cerr << "Failed to open file: " << filename << std::endl; + ATH_MSG_ERROR("Failed to open file: " << filename); return; }And for debug output:
- // Example output - for (size_t i = 0; i < energies.size(); ++i) { - std::cout << "Energy index " << i << ": " << energies[i] << " GeV\n"; - for (size_t j = 0; j < data[i].size(); ++j) { - if (!data[i][j].empty()) { - std::cout << " Layer " << j << " Shape: (" << data[i][j].size() << ", " - << data[i][j][0].size() << ")\n"; - } - } - } + ATH_MSG_DEBUG("Loaded " << energies.size() << " energy points for upscaling");Also applies to: 597-605
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/Core/TFCSBinnedShowerONNX.cxx` around lines 543 - 546, Replace the plain stream prints with the Athena logging macros: change the file-open failure message that currently uses std::cerr << "Failed to open file: " << filename to ATH_MSG_ERROR(\"Failed to open file: %s\", filename.c_str()) (or equivalent ATH_MSG_ERROR with the same text), and replace the debug prints that use std::cout in the later block with ATH_MSG_DEBUG calls preserving the original debug content; locate these in TFCSBinnedShowerONNX around the code that checks file/IsZombie() (uses variables file and filename) and the subsequent debug loop and swap each std::cout/std::cerr to the corresponding ATH_MSG_DEBUG/ATH_MSG_ERROR macro.source/Core/TFCSLateralShapeParametrizationFixedHitChain.cxx (1)
5-7: Remove unused includes.
CLHEP/Random/RandGaussZiggurat.handTMath.hare included but not used in this implementation file. Consider removing them to reduce compilation dependencies.🧹 Proposed cleanup
`#include` "FastCaloSim/Core/TFCSLateralShapeParametrizationFixedHitChain.h" -#include "CLHEP/Random/RandGaussZiggurat.h" `#include` "FastCaloSim/Core/TFCSSimulationState.h" -#include "TMath.h"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/Core/TFCSLateralShapeParametrizationFixedHitChain.cxx` around lines 5 - 7, The file currently includes unused headers; remove the two unused include lines "CLHEP/Random/RandGaussZiggurat.h" and "TMath.h" from the top of TFCSLateralShapeParametrizationFixedHitChain.cxx and keep only the required "FastCaloSim/Core/TFCSSimulationState.h" (verify no references to CLHEP::RandGaussZiggurat or TMath symbols anywhere in functions or methods of TFCSLateralShapeParametrizationFixedHitChain so the build remains clean).include/FastCaloSim/Core/TFCSBinnedShower.h (2)
112-122: Setter parameter types currently force avoidable copiesSeveral setters take large vectors by value and then copy-assign; additionally, Line 114-115 uses
constby-value parameters, which blocks move semantics entirely.Suggested fix
+#include <utility> + - void set_event_library(eventvector_t eventlibrary) + void set_event_library(eventvector_t eventlibrary) { - m_eventlibrary = eventlibrary; + m_eventlibrary = std::move(eventlibrary); } - void set_coordinates(event_bins_t coordinates) + void set_coordinates(event_bins_t coordinates) { - m_coordinates = coordinates; + m_coordinates = std::move(coordinates); } void set_sub_bin_distribution_and_energies( std::vector<std::vector<std::vector<std::vector<float>>>> sub_bin_distribution, std::vector<float> upscaling_energies) { - m_sub_bin_distribution = sub_bin_distribution; - m_upscaling_energies = upscaling_energies; + m_sub_bin_distribution = std::move(sub_bin_distribution); + m_upscaling_energies = std::move(upscaling_energies); m_use_upscaling = true; }For
set_layer_energy(...), preferconst std::vector<...>&(or non-const by-value +std::move) instead ofconstby-value parameters.Also applies to: 134-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/FastCaloSim/Core/TFCSBinnedShower.h` around lines 112 - 122, The setters currently take large std::vector parameters by-value (some marked const) which forces unnecessary copies and prevents moves; update signatures like set_layer_energy and set_bin_boundaries (and other setters in the same region) to accept either const std::vector<T>& for no-copy semantics or accept std::vector<T> by-value (remove const) and std::move into member storage inside the implementation; ensure parameter types match element types (e.g., std::vector<unsigned int>& or const std::vector<unsigned int>& for set_layer_energy and const std::vector<float>& for boundary setters) and update callers if needed.
107-109: Avoid deep copies in large-container gettersThese getters currently return large objects by value (and some are non-
constmethods), which will copy potentially massive shower data every call.Suggested fix
- const std::string get_hdf5_path() const { return m_hdf5_file; } + const std::string& get_hdf5_path() const { return m_hdf5_file; } - eventvector_t get_eventlibrary() { return m_eventlibrary; } + const eventvector_t& get_eventlibrary() const { return m_eventlibrary; } - event_bins_t get_coordinates() { return m_coordinates; } + const event_bins_t& get_coordinates() const { return m_coordinates; } - std::vector<std::vector<std::vector<std::vector<float>>>> - get_sub_bin_distribution() const + const std::vector<std::vector<std::vector<std::vector<float>>>>& + get_sub_bin_distribution() const { return m_sub_bin_distribution; } - std::vector<float> get_upscaling_energies() const + const std::vector<float>& get_upscaling_energies() const { return m_upscaling_energies; }Also applies to: 132-155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/FastCaloSim/Core/TFCSBinnedShower.h` around lines 107 - 109, The current getters (e.g., get_hdf5_path) return large objects by value causing expensive deep copies; change them to return const references (const std::string& get_hdf5_path() const) and ensure they are const-qualified, and similarly update the other large-container getters referenced (lines ~132-155) to return const T& instead of T by value; keep setters like set_hdf5_path and delete_hdf5_path unchanged but adjust any call sites that assumed copies if necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/FastCaloSim/Core/TFCSBinnedShower.h`:
- Around line 89-92: The setters set_default_hit_energy and
set_max_hits_per_voxel currently accept non-positive values; add input
validation to guard m_default_hit_energy and m_max_hits_per_voxel against <= 0.
Modify set_default_hit_energy(float energy) to check energy > 0 and throw
std::invalid_argument (or assert) with a clear message if not, and modify
set_max_hits_per_voxel(int max_hits) to check max_hits > 0 and throw
std::invalid_argument (or assert) with a clear message if not; keep the getters
unchanged. Ensure the error messages reference the setter names and the invalid
value so callers can debug easily.
In `@include/FastCaloSim/Core/TFCSBinnedShowerBase.h`:
- Line 74: The member pointer m_geo in class TFCSBinnedShowerBase is declared
uninitialized which can lead to undefined behavior; initialize it to nullptr in
its declaration and ensure any constructors or reset logic do not overwrite that
initialization, and verify methods that access it (e.g., get_geometry and
set_geometry) handle a null pointer safely or assert after set_geometry is
called.
In `@include/FastCaloSim/Core/TFCSBinnedShowerONNX.h`:
- Around line 3-4: The include guard macro is mismatched: the `#ifndef` uses
TFCSBinnedShowerONNX_h but the `#define` uses TTFCSBinnedShowerONNX_h; change the
`#define` to exactly TFCSBinnedShowerONNX_h so both macros match (and optionally
update the trailing `#endif` comment to reference TFCSBinnedShowerONNX_h if
present). Ensure no other occurrences of TTFCSBinnedShowerONNX_h remain in the
file.
In `@include/FastCaloSim/Core/TFCSPhiModulationCorrection.h`:
- Line 119: The member pointer m_geo in class TFCSPhiModulationCorrection is not
initialized which can lead to undefined behaviour if accessed before
set_geometry(); update the declaration of ICaloGeometry* m_geo in
TFCSPhiModulationCorrection so it is initialized to nullptr (matching
TFCSBinnedShowerBase style) to ensure a safe default and avoid
uninitialized-pointer use.
In `@include/FastCaloSim/LinkDef.h`:
- Around line 45-49: The LinkDef.h is missing ROOT dictionary registrations for
classes that declare ClassDefOverride; add corresponding "#pragma link C++
class" entries for TFCSPhiModulationCorrection, TFCSBinnedShowerBase,
TFCSBinnedShower, TFCSBinnedShowerONNX, TFCSMLCalorimeterSimulator,
TFCSLateralShapeParametrizationFixedHitChain, and
TFCSVoxelHistoLateralCovarianceFluctuations so ROOT generates
dictionaries/streamers for them; locate the existing LinkDef.h pragma block and
append one pragma line per class (ensure proper trailing '+' or nothing per
project convention) matching the exact class names above.
In `@source/Core/TFCSBinnedShower.cxx`:
- Around line 195-220: The code reads
simulstate.getAuxInfo<int>("EventNr"_FCShash) into event_index and then uses
m_eventlibrary.at(event_index) without validating bounds; add a guard after
retrieving EventNr (inside the simulstate.hasAuxInfo && m_use_event_matching
branch) to verify the value is >= 0 and <
static_cast<int>(m_eventlibrary.size()), otherwise replace event_index with a
safe fallback (e.g., sample a random valid index using
CLHEP::RandFlat::shoot(simulstate.randomEngine(), 0, m_eventlibrary.size())) and
log a warning via ATH_MSG_WARN indicating the aux EventNr was out of range;
ensure event_index remains the same unsigned/size_t type used later or
explicitly cast after validation to avoid signed/unsigned conversion issues.
In `@source/Core/TFCSBinnedShowerONNX.cxx`:
- Around line 381-410: Add an explicit bounds check for bin_index before any
.at(bin_index) access on m_sub_bin_distribution entries (the branches that read
m_sub_bin_distribution.at(e_index).at(layer_index).at(bin_index) and the
interpolation branch that reads .at(e_index-1).at(layer_index).at(bin_index) and
.at(e_index).at(layer_index).at(bin_index)), e.g. verify
m_sub_bin_distribution.at(...).at(layer_index).size() > bin_index (or otherwise
assert/document precondition) and handle the out-of-range case (return
default/throw a clearer error/skip interpolation) so probabilities access is
safe for the single-energy and multi-energy paths that use available_energies,
e_index, layer_index and bin_index.
- Around line 467-469: The code casts aux info to pointers (event and
hits_per_layer_ptr) without checking for null, which can crash if the aux info
is missing; update the TFCSBinnedShowerONNX.cxx code around the static_cast of
TFCSMLCalorimeterSimulator::event_t* event and the hits_per_layer_ptr retrieval
to check the result of simulstate.getAuxInfo<void*>("BSEventData"_FCShash) and
the corresponding aux key for hits before dereferencing, and handle the null
case by logging a descriptive error via the existing logger and early-return (or
skip processing) from the function so no dereference occurs.
- Around line 210-213: The code obtains TFCSMLCalorimeterSimulator::event_t*
event via simulstate.getAuxInfo<void*>("BSEventData"_FCShash) but never checks
for null before use; update the code around the event variable (the result of
simulstate.getAuxInfo<void*>("BSEventData"_FCShash) stored in event) to verify
event != nullptr and handle the failure path (e.g. log an error via the
appropriate logger and return/skip processing) before any dereference, ensuring
existing logic that uses event and e_init remains unchanged when event is valid.
In `@source/Core/TFCSLateralShapeParametrizationFixedHitChain.cxx`:
- Around line 38-44: The code reads
simulstate.getAuxInfo<int>("FCSHitChainNHits"_FCShash) without checking
existence, which can throw if the aux key is missing; update
TFCSLateralShapeParametrizationFixedHitChain:: method to call
simulstate.hasAuxInfo("FCSHitChainNHits"_FCShash) before invoking getAuxInfo,
and if it returns false log an error via ATH_MSG_ERROR (similar wording to the
existing nhit_signed check) and return true to abort; keep the existing
nhit_signed < 0 handling unchanged and use the same variable name (nhit_signed)
when reading the value after the existence check.
In `@source/Core/TFCSMLCalorimeterSimulator.cxx`:
- Around line 94-115: The current logic only inspects outputs.begin()->second
and can dereference an empty VNetworkBase::NetworkOutputs and miss NaNs in other
elements; update the post-call validation after calling
predictVoxels(simulstate, eta, energy) to first check outputs.empty() and then
iterate all entries in outputs to verify none are NaN (e.g., check each
pair.second with std::isnan), retrying the full predictVoxels call up to 5 times
if either the output is empty or any value is NaN, and only proceed when the
entire outputs map passes these validations (log warnings and give up after the
retry budget as before).
- Around line 84-85: Both overloads of predictVoxels() call
m_onnx_model->compute(...) without checking that the model is loaded; add a
guard that tests m_onnx_model (e.g. if (!m_onnx_model) ...) before any compute()
call in predictVoxels() and in any other place that calls compute(), and handle
the missing-model case by returning an appropriate failure/empty result and
logging an error (do not dereference m_onnx_model). Update both compute call
sites (the one around "VNetworkBase::NetworkOutputs outputs =
m_onnx_model->compute(inputs);" and the second occurrence noted at line ~200) to
use this guard so the function fails cleanly when loadSimulator() has not been
called or failed.
- Around line 134-145: The code reads voxel outputs using
outputs[std::to_string(voxel_index)] which mismatches VNetworkBase's key
convention (node names or node_name_part_n) and also silently inserts zeros;
change the lookup to construct the correct key (e.g. key = m_output_node_name +
"_part_" + std::to_string(voxel_index) or follow the actual node name pattern
used by the network) and use outputs.find(key) (or outputs.at(key) with
try/catch) to check existence before using the value; replace the current
voxel_energy assignment and subsequent logic around outputs, voxel_index, and
E_vector (in TFCSMLCalorimeterSimulator.cxx) so missing keys are not inserted
and only present outputs are pushed into event.event_data and E_vector (divide
by energy as before).
In `@source/Core/TFCSPhiModulationCorrection.cxx`:
- Around line 190-192: The code calls m_geo->getDDE(...) and then uses
cellele->phi() without verifying pointers; add null checks: verify m_geo is
non-null before calling m_geo->getDDE(layer_index, eta, phi) and check the
returned CaloDetDescrElement* (cellele) for null before accessing cellele->phi()
(and handle the error path or return/continue accordingly) to avoid
dereferencing null pointers.
- Around line 119-142: The code uses TFile::Open and
muon_corr->Get("hWt_Layer0") without null checks which can crash; add checks
after TFile::Open(filename.c_str()) to verify muon_corr is not nullptr and
return/continue with a clear error if it failed, then retrieve muon_corr_hist
via muon_corr->Get("hWt_Layer0") and check muon_corr_hist is not nullptr before
calling GetNbinsX(), GetXaxis(), GetBinContent(), or accessing bin edges; only
call muon_corr->Get("energy_shift") and assign energy_shifts.at(eta_index) after
verifying muon_corr is valid, and ensure muon_corr->Close() is called only if
muon_corr was successfully opened.
- Around line 361-367: The removal path in remove_phi_modulation (using
m_modulation in remove_phi_modulation) doesn't undo the energy_shift applied in
add_phi_modulation; update remove_phi_modulation to also divide by the same
energy_shift factor (m_modulation_scale or the symbol used to compute
energy_shift in add_phi_modulation) so
remove_phi_modulation(add_phi_modulation(E)) == E, or if asymmetry is deliberate
add a comment in remove_phi_modulation explaining why energy_shift is not
inverted; locate functions add_phi_modulation and remove_phi_modulation and make
the change to apply the reciprocal of the exact energy_shift expression used in
add_phi_modulation.
In `@source/Core/TFCSVoxelHistoLateralCovarianceFluctuations.cxx`:
- Around line 252-253: The current computation of cdf_val using
TMath::Erf(genPars[count] * 2.0 / TMath::Pi()) is incorrect for the standard
normal CDF; replace that expression with the proper normal CDF mapping, e.g.
cdf_val = 0.5 * (1.0 + TMath::Erf(genPars[count] / TMath::Sqrt(2.0))); update
the line that sets cdf_val (referencing cdf_val and genPars[count] and
TMath::Erf) accordingly so sampled voxel weights are not biased.
- Around line 76-83: The template histograms returned by inputfile->Get(...) are
owned by the TFile and will be deleted when the file is closed, so do not store
those pointers directly in m_voxel_template; instead, after verifying temp is
non-null, create an independent copy (e.g., via
dynamic_cast<TH2*>(temp->Clone()) ), detach it from any file-managed directory
(call SetDirectory(nullptr) or equivalent), push the cloned histogram into
m_voxel_template, and then use the clone to set m_nDim_x and later in
simulate(); update the code around inputfile->Get/Form("voxel_template_cs...")
and any uses of m_voxel_template[0] to work with the cloned, file-independent
TH2 objects (class TFCSVoxelHistoLateralCovarianceFluctuations, members
m_voxel_template and simulate).
- Around line 229-245: The code only checks Ebin <= 0 but later indexes
m_parMeans[Ebin - 1], m_transform[Ebin - 1], m_EigenValues[Ebin - 1], and
m_EigenVectors[Ebin - 1]; add a bounds check against the loaded PCA tables (e.g.
ensure Ebin > 0 && Ebin <= static_cast<int>(m_parMeans.size()) or the
appropriate container size) and return FCSError/FCSSuccess as appropriate if out
of range, making all subsequent accesses use Ebin - 1 only after this validation
so no out-of-bounds indexing occurs.
- Line 391: The code attempts to modify energy via the const accessor hit.E()
(which returns a const reference) causing a compilation error; replace the
in-place modification with a read-modify-write using the setter: retrieve the
energy value from hit.E(), multiply by weight, and call hit.set_E(...) to store
the scaled energy (i.e., compute newEnergy = hit.E() * weight and call
hit.set_E(newEnergy) in TFCSVoxelHistoLateralCovarianceFluctuations where
hit.E() *= weight is used).
---
Outside diff comments:
In `@include/FastCaloSim/Core/TFCSLateralShapeParametrizationHitBase.h`:
- Around line 132-145: m_hit_index is never initialized or cleared, so callers
of idx() can read garbage; initialize m_hit_index in the
TFCSLateralShapeParametrizationHitBase constructor (set to a sentinel like ~0u
or 0), ensure reset() clears it back to the sentinel, and expose a controlled
mutator (e.g., set_idx(unsigned long) or assign it where hits are created) so
code using idx() always reads a well-defined value; update uses in methods that
construct/populate hits to call the new setter or assign m_hit_index during
construction/initialization (referencing m_hit_index, idx(), constructor of
TFCSLateralShapeParametrizationHitBase, and reset()).
---
Nitpick comments:
In `@include/FastCaloSim/Core/TFCSBinnedShower.h`:
- Around line 112-122: The setters currently take large std::vector parameters
by-value (some marked const) which forces unnecessary copies and prevents moves;
update signatures like set_layer_energy and set_bin_boundaries (and other
setters in the same region) to accept either const std::vector<T>& for no-copy
semantics or accept std::vector<T> by-value (remove const) and std::move into
member storage inside the implementation; ensure parameter types match element
types (e.g., std::vector<unsigned int>& or const std::vector<unsigned int>& for
set_layer_energy and const std::vector<float>& for boundary setters) and update
callers if needed.
- Around line 107-109: The current getters (e.g., get_hdf5_path) return large
objects by value causing expensive deep copies; change them to return const
references (const std::string& get_hdf5_path() const) and ensure they are
const-qualified, and similarly update the other large-container getters
referenced (lines ~132-155) to return const T& instead of T by value; keep
setters like set_hdf5_path and delete_hdf5_path unchanged but adjust any call
sites that assumed copies if necessary.
In `@include/FastCaloSim/Core/TFCSBinnedShowerBase.h`:
- Line 11: The header TFCSBinnedShowerBase.h currently includes <fstream> which
is unused; remove the unused include directive from the top of
TFCSBinnedShowerBase.h (the line "#include <fstream>") to clean up includes and
avoid unnecessary dependencies, then run a quick rebuild to ensure no
compilation errors.
- Around line 103-106: Remove the unused private member m_n_layers from class
TFCSBinnedShowerBase: delete the declaration "const long unsigned int m_n_layers
= CaloCell_ID_FCS::CaloSample_FCS::MaxSample;" from TFCSBinnedShowerBase.h and
remove any references/usages in TFCSBinnedShowerBase.cxx (if present), using
CaloCell_ID_FCS::CaloSample_FCS::MaxSample directly where needed; leave
ClassDefOverride(TFCSBinnedShowerBase, 1) intact and ensure headers/compilation
still succeed after the cleanup.
In `@include/FastCaloSim/Core/TFCSBinnedShowerONNX.h`:
- Line 143: Comment typo in TFCSBinnedShowerONNX.h: change the spelling
"mamimum" to "maximum" in the comment that reads "What is the mamimum number of
hits per voxel (for runtime reasons)"; update the comment text where this
appears (e.g., near the declaration/usage of TFCSBinnedShowerONNX-related member
or constant) so it reads "What is the maximum number of hits per voxel (for
runtime reasons)".
- Around line 53-58: Remove the outdated TODO in
TFCSBinnedShowerONNX::load_simulator that asks about deleting m_ai_simulator;
keep the existing allocation code (m_ai_simulator = new
TFCSMLCalorimeterSimulator(); m_ai_simulator->loadSimulator(filename);) as the
destructor in TFCSBinnedShowerONNX (which deletes and nulls m_ai_simulator)
already handles cleanup, so simply delete the comment line and leave the method
implementation unchanged.
In `@include/FastCaloSim/Core/TFCSMLCalorimeterSimulator.h`:
- Around line 24-33: Replace the C-style typedef structs with idiomatic C++
struct declarations: change the `typedef struct { std::vector<unsigned int>
bin_index_vector; std::vector<float> E_vector; } layer_t;` to a plain `struct
layer_t { ... };` and likewise change `typedef struct { std::vector<layer_t>
event_data; } event_t;` to `struct event_t { ... };`. Update any forward
references or includes if needed, but keep the same type names (`layer_t`,
`event_t`) and member names (`bin_index_vector`, `E_vector`, `event_data`) so
usage in functions and classes (e.g., anywhere referencing layer_t or event_t)
remains unchanged.
- Line 72: The include guard closing comment in TFCSMLCalorimeterSimulator.h is
incorrect; update the end-of-file comment to match the actual guard name
TFCSMLCALORIMETERSIMULATOR_H (replace ISF_TFCSMLCALORIMETERSIMULATOR_H with
TFCSMLCALORIMETERSIMULATOR_H) so the `#endif` comment corresponds to the
`#ifndef/`#define at the top of TFCSMLCalorimeterSimulator.h.
In `@include/FastCaloSim/Core/TFCSPhiModulationCorrection.h`:
- Around line 42-54: The getters get_phi_modulation(), get_min_eta(),
get_energy_shift(), and get_min_phi() currently return large nested std::vector
by value causing unnecessary copies; change their signatures in
TFCSPhiModulationCorrection to return const references (e.g. const
std::vector<std::vector<std::vector<float>>>& get_phi_modulation() const) and
similarly for get_min_eta(), get_energy_shift(), and get_min_phi(), ensuring the
returned types match the member types (m_modulation, m_min_eta, m_energy_shift,
m_min_phi) and maintain const-correctness so callers cannot modify internal
state; update any call sites if they relied on receiving a copy.
In `@source/Core/TFCSBinnedShowerBase.cxx`:
- Around line 76-79: Replace the redundant reset+add on the simulstate energy by
using a single assignment: remove the simulstate.set_E(layer_index, 0) followed
by simulstate.add_E(layer_index, layer_energy) and instead call
simulstate.set_E(layer_index, layer_energy) so the energy for the given
layer_index is set directly to layer_energy; update references to
simulstate.set_E and simulstate.add_E in this block accordingly.
- Around line 9-12: In TFCSBinnedShowerBase.cxx (TFCSBinnedShowerBase
implementation) remove unused header includes H5Cpp.h, TFile.h, TH2.h, TBuffer.h
and TClass.h from the top of the file, leaving only the headers actually used
(e.g. CLHEP/Random/RanluxEngine.h and any TFCS* headers referenced by functions
in the TFCSBinnedShowerBase class); ensure compilation still passes and add any
removed headers back only in derived classes that require HDF5, ROOT TFile/TH2
or ROOT streaming support.
In `@source/Core/TFCSBinnedShowerONNX.cxx`:
- Around line 514-516: In TFCSBinnedShowerONNX::delete_event, replace the
ATH_MSG_ERROR calls that report missing aux data (the three occurrences flagged)
with less aggressive logging or no log: either use ATH_MSG_DEBUG (preferred if
missing aux data can be normal) or ATH_MSG_WARNING (if it’s unexpected but
non-fatal), or simply omit the message when the aux pointer is already null;
update the three error sites inside delete_event to follow this change so
spurious errors are not emitted when initialization was skipped or failed.
- Around line 543-546: Replace the plain stream prints with the Athena logging
macros: change the file-open failure message that currently uses std::cerr <<
"Failed to open file: " << filename to ATH_MSG_ERROR(\"Failed to open file:
%s\", filename.c_str()) (or equivalent ATH_MSG_ERROR with the same text), and
replace the debug prints that use std::cout in the later block with
ATH_MSG_DEBUG calls preserving the original debug content; locate these in
TFCSBinnedShowerONNX around the code that checks file/IsZombie() (uses variables
file and filename) and the subsequent debug loop and swap each
std::cout/std::cerr to the corresponding ATH_MSG_DEBUG/ATH_MSG_ERROR macro.
In `@source/Core/TFCSLateralShapeParametrizationFixedHitChain.cxx`:
- Around line 5-7: The file currently includes unused headers; remove the two
unused include lines "CLHEP/Random/RandGaussZiggurat.h" and "TMath.h" from the
top of TFCSLateralShapeParametrizationFixedHitChain.cxx and keep only the
required "FastCaloSim/Core/TFCSSimulationState.h" (verify no references to
CLHEP::RandGaussZiggurat or TMath symbols anywhere in functions or methods of
TFCSLateralShapeParametrizationFixedHitChain so the build remains clean).
In `@source/Core/TFCSPhiModulationCorrection.cxx`:
- Line 139: Replace the direct std::cout debug print with the ATLAS logging
macro: use ATH_MSG_DEBUG to log the energy shift instead of std::cout in
TFCSPhiModulationCorrection (the location that prints
energy_shifts.at(eta_index)); format the message similarly (e.g. a descriptive
string plus the energy_shifts.at(eta_index) value) so it integrates with the
component's logger and follows the usual ATH_MSG_* conventions.
🪄 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: 0a33b2ac-2efb-430b-b46b-19fc4e2d610f
📒 Files selected for processing (17)
include/FastCaloSim/Core/TFCSBinnedShower.hinclude/FastCaloSim/Core/TFCSBinnedShowerBase.hinclude/FastCaloSim/Core/TFCSBinnedShowerONNX.hinclude/FastCaloSim/Core/TFCSLateralShapeParametrizationFixedHitChain.hinclude/FastCaloSim/Core/TFCSLateralShapeParametrizationHitBase.hinclude/FastCaloSim/Core/TFCSLateralShapeParametrizationHitChain.hinclude/FastCaloSim/Core/TFCSMLCalorimeterSimulator.hinclude/FastCaloSim/Core/TFCSPhiModulationCorrection.hinclude/FastCaloSim/Core/TFCSVoxelHistoLateralCovarianceFluctuations.hinclude/FastCaloSim/LinkDef.hsource/Core/TFCSBinnedShower.cxxsource/Core/TFCSBinnedShowerBase.cxxsource/Core/TFCSBinnedShowerONNX.cxxsource/Core/TFCSLateralShapeParametrizationFixedHitChain.cxxsource/Core/TFCSMLCalorimeterSimulator.cxxsource/Core/TFCSPhiModulationCorrection.cxxsource/Core/TFCSVoxelHistoLateralCovarianceFluctuations.cxx
Comment on lines
+89
to
+92
| void set_default_hit_energy(float energy) { m_default_hit_energy = energy; } | ||
| float get_default_hit_energy() const { return m_default_hit_energy; } | ||
| void set_max_hits_per_voxel(int max_hits) { m_max_hits_per_voxel = max_hits; } | ||
| int get_max_hits_per_voxel() const { return m_max_hits_per_voxel; } |
There was a problem hiding this comment.
Add validation for hit-energy and hit-count limits
Line 89 and Line 91 accept invalid values (<= 0), which can put the object into an unsafe runtime state (e.g., divide-by-zero or non-sensical hit limits downstream).
Suggested fix
+#include <stdexcept>
+
- void set_default_hit_energy(float energy) { m_default_hit_energy = energy; }
+ void set_default_hit_energy(float energy)
+ {
+ if (energy <= 0.F) {
+ throw std::invalid_argument("default hit energy must be > 0");
+ }
+ m_default_hit_energy = energy;
+ }
float get_default_hit_energy() const { return m_default_hit_energy; }
- void set_max_hits_per_voxel(int max_hits) { m_max_hits_per_voxel = max_hits; }
+ void set_max_hits_per_voxel(int max_hits)
+ {
+ if (max_hits <= 0) {
+ throw std::invalid_argument("max hits per voxel must be > 0");
+ }
+ m_max_hits_per_voxel = max_hits;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/FastCaloSim/Core/TFCSBinnedShower.h` around lines 89 - 92, The
setters set_default_hit_energy and set_max_hits_per_voxel currently accept
non-positive values; add input validation to guard m_default_hit_energy and
m_max_hits_per_voxel against <= 0. Modify set_default_hit_energy(float energy)
to check energy > 0 and throw std::invalid_argument (or assert) with a clear
message if not, and modify set_max_hits_per_voxel(int max_hits) to check
max_hits > 0 and throw std::invalid_argument (or assert) with a clear message if
not; keep the getters unchanged. Ensure the error messages reference the setter
names and the invalid value so callers can debug easily.
There was a problem hiding this comment.
🧩 Analysis chain
Let me first check the base class implementation to understand the existing validation style before planning the fix.
🏁 Script executed:
#!/bin/bash
rg -n "set_default_hit_energy|set_max_hits_per_voxel" --type cpp -C3
Length of output: 1026
@floern1, good news — the base class TFCSBinnedShowerBase already has these setters with assert guards:
// TFCSBinnedShowerBase.h (lines 40–49)
void set_default_hit_energy(float energy)
{
assert(energy > 0.F && "default hit energy must be > 0");
m_default_hit_energy = energy;
}
void set_max_hits_per_voxel(int max_hits)
{
assert(max_hits > 0 && "max hits per voxel must be > 0");
m_max_hits_per_voxel = max_hits;
}TFCSBinnedShower.h re-declares the same setters and getters without guards, which hides the base class implementations and silently bypasses the assert checks. The cleanest fix is to simply remove the redundant declarations from the derived class:
- Remove the four unguarded setter/getter methods from
TFCSBinnedShowerthat are already correctly defined (withassertguards) inTFCSBinnedShowerBase
The requested changes have been implemented and a pull request has been created: View PR
Comment on lines
+361
to
+367
| const std::vector<float>& modulation = | ||
| m_modulation.at(layer_index).at(eta_index); | ||
|
|
||
| energy /= ((modulation.at(phi_index) - 1) * m_modulation_scale + 1); | ||
|
|
||
| return energy; | ||
| } |
There was a problem hiding this comment.
remove_phi_modulation does not reverse energy_shift applied in add_phi_modulation.
In add_phi_modulation (lines 296-297), the energy is multiplied by both the modulation factor and energy_shift. However, remove_phi_modulation (line 364) only divides by the modulation factor, not by energy_shift. This asymmetry means remove_phi_modulation(add_phi_modulation(E)) != E.
If this is intentional, please add a comment explaining why. Otherwise, consider adding the missing division.
🐛 Proposed fix if symmetry is intended
energy /= ((modulation.at(phi_index) - 1) * m_modulation_scale + 1);
+ energy /= m_energy_shift.at(layer_index).at(eta_index);
return energy;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const std::vector<float>& modulation = | |
| m_modulation.at(layer_index).at(eta_index); | |
| energy /= ((modulation.at(phi_index) - 1) * m_modulation_scale + 1); | |
| return energy; | |
| } | |
| const std::vector<float>& modulation = | |
| m_modulation.at(layer_index).at(eta_index); | |
| energy /= ((modulation.at(phi_index) - 1) * m_modulation_scale + 1); | |
| energy /= m_energy_shift.at(layer_index).at(eta_index); | |
| return energy; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/Core/TFCSPhiModulationCorrection.cxx` around lines 361 - 367, The
removal path in remove_phi_modulation (using m_modulation in
remove_phi_modulation) doesn't undo the energy_shift applied in
add_phi_modulation; update remove_phi_modulation to also divide by the same
energy_shift factor (m_modulation_scale or the symbol used to compute
energy_shift in add_phi_modulation) so
remove_phi_modulation(add_phi_modulation(E)) == E, or if asymmetry is deliberate
add a comment in remove_phi_modulation explaining why energy_shift is not
inverted; locate functions add_phi_modulation and remove_phi_modulation and make
the change to apply the reciprocal of the exact energy_shift expression used in
add_phi_modulation.
Collaborator
There was a problem hiding this comment.
The phi modulation is extracted by fitting a free floating histogram to very finely binned (in phi) G4 voxels. When removing the phi modulation by dividing it out and reapplying it after a random phi shift, a mean shift in the total energy might arise that biases the simulation prediction. This mean shift can be captured in the the correction curve training and has to be applied to prevent this bias. Therefore, it is asymmetric.
There was a problem hiding this comment.
@floern1, thanks for the clear explanation! That makes sense — the energy_shift compensates for a mean energy bias that arises from the remove-then-reapply cycle, and this correction is baked in during training, so it should only be applied in the add direction.
To prevent future confusion (or future reviewers flagging this again), it would be very helpful to add a brief comment at the division site in remove_phi_modulation documenting this intentional asymmetry:
energy /= ((modulation.at(phi_index) - 1) * m_modulation_scale + 1);
// Note: energy_shift is intentionally NOT inverted here.
// It corrects a mean energy bias that arises when phi modulation is removed
// and then reapplied at a different (random) phi. This bias is captured
// during correction curve training and must only be applied in add_phi_modulation.
return energy;✏️ Learnings added
Learnt from: floern1
Repo: fcs-proj/FastCaloSim PR: 84
File: source/Core/TFCSPhiModulationCorrection.cxx:366-372
Timestamp: 2026-05-08T09:40:48.015Z
Learning: In `source/Core/TFCSPhiModulationCorrection.cxx`, the asymmetry between `add_phi_modulation` and `remove_phi_modulation` regarding `energy_shift` is intentional. `add_phi_modulation` applies both the modulation factor and `energy_shift`, while `remove_phi_modulation` only divides by the modulation factor. The `energy_shift` corrects a mean energy bias that arises when phi modulation is removed and reapplied at a random phi shift — this bias is captured during the correction curve training and must only be applied in the `add` direction.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Comment on lines
+76
to
+83
| TH2* temp = dynamic_cast<TH2*>( | ||
| inputfile->Get(Form("voxel_template_cs%d_pca%d", cs, bin))); | ||
| if (!temp) { | ||
| ATH_MSG_ERROR("Template hist not found for cs " + std::to_string(cs)); | ||
| return false; | ||
| } | ||
| m_voxel_template.push_back(temp); | ||
| m_nDim_x = m_voxel_template[0]->GetNbinsX(); |
There was a problem hiding this comment.
Do not retain a TFile-owned template histogram past initialization.
inputfile->Get() returns an object owned by the file. m_voxel_template[0] is used again in simulate() on Line 267, so once the file is closed this becomes a dangling pointer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/Core/TFCSVoxelHistoLateralCovarianceFluctuations.cxx` around lines 76
- 83, The template histograms returned by inputfile->Get(...) are owned by the
TFile and will be deleted when the file is closed, so do not store those
pointers directly in m_voxel_template; instead, after verifying temp is
non-null, create an independent copy (e.g., via
dynamic_cast<TH2*>(temp->Clone()) ), detach it from any file-managed directory
(call SetDirectory(nullptr) or equivalent), push the cloned histogram into
m_voxel_template, and then use the clone to set m_nDim_x and later in
simulate(); update the code around inputfile->Get/Form("voxel_template_cs...")
and any uses of m_voxel_template[0] to work with the cloned, file-independent
TH2 objects (class TFCSVoxelHistoLateralCovarianceFluctuations, members
m_voxel_template and simulate).
Comment on lines
+252
to
+253
| double cdf_val = | ||
| (TMath::Erf(genPars[count] * 2.0 / TMath::Pi()) + 1) / 2.0; |
There was a problem hiding this comment.
Use the standard normal CDF here.
0.5 * (1 + erf(x * 2 / pi)) is not the Gaussian CDF. If the intent is to map a Gaussian draw to a uniform CDF value, this should be 0.5 * (1 + erf(x / sqrt(2))); the current scaling biases every sampled voxel weight.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/Core/TFCSVoxelHistoLateralCovarianceFluctuations.cxx` around lines 252
- 253, The current computation of cdf_val using TMath::Erf(genPars[count] * 2.0
/ TMath::Pi()) is incorrect for the standard normal CDF; replace that expression
with the proper normal CDF mapping, e.g. cdf_val = 0.5 * (1.0 +
TMath::Erf(genPars[count] / TMath::Sqrt(2.0))); update the line that sets
cdf_val (referencing cdf_val and genPars[count] and TMath::Erf) accordingly so
sampled voxel weights are not biased.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
include/FastCaloSim/Core/TFCSPhiModulationCorrection.h (1)
119-119:⚠️ Potential issue | 🟠 MajorInitialize
m_geoto a safe default.Line 119 leaves
m_geouninitialized; dereferencing it before Line 104-108 runs is undefined behavior.🛡️ Proposed fix
- CaloGeo* m_geo; //! do not persistify + CaloGeo* m_geo = nullptr; //! do not persistify🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/FastCaloSim/Core/TFCSPhiModulationCorrection.h` at line 119, m_geo is declared uninitialized (CaloGeo* m_geo) which can lead to UB if dereferenced before it's set; initialize it to a safe default (nullptr) by changing the member declaration or adding it to the class constructor's initializer list for TFCSPhiModulationCorrection so m_geo starts as nullptr until properly assigned, and ensure any code that dereferences m_geo checks for nullptr first (reference symbol: m_geo and class/constructors named TFCSPhiModulationCorrection).include/FastCaloSim/Core/TFCSBinnedShowerBase.h (1)
74-74:⚠️ Potential issue | 🟡 MinorInitialize
m_geotonullptrto avoid undefined behavior.On Line 74,
m_geois left uninitialized, soget_geometry()can return garbage beforeset_geometry()is called.Suggested fix
-protected: - CaloGeo* m_geo; //! do not persistify +protected: + CaloGeo* m_geo = nullptr; //! do not persistify🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/FastCaloSim/Core/TFCSBinnedShowerBase.h` at line 74, m_geo is uninitialized which can make get_geometry() return garbage; initialize the member pointer to nullptr by setting CaloGeo* m_geo = nullptr (or explicitly initialize m_geo in the TFCSBinnedShowerBase constructor) so get_geometry() is safe before set_geometry() is called; update the declaration of m_geo and/or the class constructor(s) (TFCSBinnedShowerBase) accordingly.
🧹 Nitpick comments (1)
include/FastCaloSim/Core/TFCSPhiModulationCorrection.h (1)
28-32: Take care with public API changes affecting return types and parameter passing.No internal callers of the getter methods were found in the codebase, making the proposed refactoring safe from an internal perspective. However, these are public methods and the class is exported for ROOT serialization (LinkDef.h), which means external code or serialized objects may depend on the current copy semantics. Changing return types from value to
const&and parameters toconst std::string&would break compatibility with external callers that expect copies. Verify that this API change is acceptable for your project's stability guarantees before applying it.The
load_phi_modulationparameter change toconst std::string&is safe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/FastCaloSim/Core/TFCSPhiModulationCorrection.h` around lines 28 - 32, The review warns that changing public getter return types and parameter passing could break external users and ROOT serialization; keep the existing public copy semantics for the getter methods in class TFCSPhiModulationCorrection (do not change their return types from value to const&) to preserve ABI/serialization compatibility (LinkDef.h), but you may change load_phi_modulation's signature to accept const std::string& filename and also make its parameter layer_index/eta_min/eta_max/energy_shift const where appropriate; if you truly need to change getters to return const&, first confirm project stability guarantees and update serialization/LinkDef and any external consumers accordingly before applying that refactor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/FastCaloSim/Core/TFCSBinnedShowerBase.h`:
- Around line 88-91: Update the stale comment for
get_layer_energy(TFCSSimulationState& simulstate, long unsigned int layer_index)
const to remove any reference to “position” and clearly state that the function
returns the energy (as a float) of the hit in the specified layer for the given
simulation state; mention the meaning of the parameters (simulstate and
layer_index) and that the returned value is the layer energy (include units if
applicable) so implementers are not misled.
- Around line 62-71: The conversion from unsigned long to int in
get_number_of_hits() (using nhits = get_n_hits(simulstate, calosample())) can
overflow; update get_number_of_hits() to check for nhits == 0 (keep returning
-1) and then guard against values larger than std::numeric_limits<int>::max()
(or another sentinel) — e.g., if nhits > max int return -1 or clamp—before doing
static_cast<int>(nhits), ensuring get_n_hits, calosample and TFCSSimulationState
usage remains unchanged.
- Around line 100-101: simulate() calls get_event() but never invokes
delete_event(), leaking per-event heap allocations; ensure
delete_event(TFCSSimulationState&) is called once per event after all hit
processing completes by adding a cleanup call in simulate() (or wrapping
simulstate in an RAII guard) so TFCSBinnedShower::delete_event and
TFCSBinnedShowerONNX::delete_event run reliably; reference the get_event /
simulate calls in TFCSBinnedShowerBase and the delete_event virtual to locate
where to insert the call or construct the RAII wrapper that invokes delete_event
in its destructor.
---
Duplicate comments:
In `@include/FastCaloSim/Core/TFCSBinnedShowerBase.h`:
- Line 74: m_geo is uninitialized which can make get_geometry() return garbage;
initialize the member pointer to nullptr by setting CaloGeo* m_geo = nullptr (or
explicitly initialize m_geo in the TFCSBinnedShowerBase constructor) so
get_geometry() is safe before set_geometry() is called; update the declaration
of m_geo and/or the class constructor(s) (TFCSBinnedShowerBase) accordingly.
In `@include/FastCaloSim/Core/TFCSPhiModulationCorrection.h`:
- Line 119: m_geo is declared uninitialized (CaloGeo* m_geo) which can lead to
UB if dereferenced before it's set; initialize it to a safe default (nullptr) by
changing the member declaration or adding it to the class constructor's
initializer list for TFCSPhiModulationCorrection so m_geo starts as nullptr
until properly assigned, and ensure any code that dereferences m_geo checks for
nullptr first (reference symbol: m_geo and class/constructors named
TFCSPhiModulationCorrection).
---
Nitpick comments:
In `@include/FastCaloSim/Core/TFCSPhiModulationCorrection.h`:
- Around line 28-32: The review warns that changing public getter return types
and parameter passing could break external users and ROOT serialization; keep
the existing public copy semantics for the getter methods in class
TFCSPhiModulationCorrection (do not change their return types from value to
const&) to preserve ABI/serialization compatibility (LinkDef.h), but you may
change load_phi_modulation's signature to accept const std::string& filename and
also make its parameter layer_index/eta_min/eta_max/energy_shift const where
appropriate; if you truly need to change getters to return const&, first confirm
project stability guarantees and update serialization/LinkDef and any external
consumers accordingly before applying that refactor.
🪄 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: fc0e0d16-1321-4727-8124-d4293dd01e8e
📒 Files selected for processing (2)
include/FastCaloSim/Core/TFCSBinnedShowerBase.hinclude/FastCaloSim/Core/TFCSPhiModulationCorrection.h
Comment on lines
+100
to
+101
| // Delete all pointers that were created in get_event() | ||
| virtual void delete_event(TFCSSimulationState& simulstate) const = 0; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== delete_event declarations/overrides/calls =="
rg -nP -C3 '\bdelete_event\s*\('
echo
echo "== get_event declarations/overrides/calls =="
rg -nP -C3 '\bget_event\s*\('
echo
echo "== TFCSBinnedShowerBase simulate/simulate_hit bodies =="
rg -nP -C20 'TFCSBinnedShowerBase::simulate(_hit)?\s*\('Repository: fcs-proj/FastCaloSim
Length of output: 13936
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== All delete_event() call sites (invocations) =="
rg -n 'delete_event\s*\(' --type cpp --type h | grep -v 'virtual\|override\|=\s*0'
echo
echo "== Complete simulate() method in TFCSBinnedShowerBase.cxx =="
sed -n '36,200p' source/Core/TFCSBinnedShowerBase.cxx | cat -n
echo
echo "== Check for RAII patterns or cleanup guards =="
rg -n 'make_unique|make_shared|unique_ptr|shared_ptr|RAII' source/Core/TFCSBinnedShowerBase.cxxRepository: fcs-proj/FastCaloSim
Length of output: 6064
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Calls to simulate() on TFCSBinnedShowerBase or derived types =="
rg -n '\.simulate\s*\(' --type cpp -A3 -B1 | head -60
echo
echo "== Files that import/include TFCSBinnedShowerBase =="
rg -l 'TFCSBinnedShowerBase' --type cpp --type h | head -20
echo
echo "== Check base class TFCSLateralShapeParametrizationHitBase =="
rg -n 'class TFCSLateralShapeParametrizationHitBase' --type h -A20Repository: fcs-proj/FastCaloSim
Length of output: 130
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Base class TFCSLateralShapeParametrizationHitBase =="
find . -name "*.h" -o -name "*.cxx" | xargs grep -l 'class TFCSLateralShapeParametrizationHitBase'
echo
echo "== Look at TFCSLateralShapeParametrizationHitBase header =="
find . -name "*TFCSLateralShapeParametrizationHitBase*" -type f | head -5
echo
echo "== Broader search for simulate() callers (look for framework/runner) =="
rg -n 'simulate' source/ include/ --type cpp --type h -l | grep -i 'runner\|framework\|sim\|event' | head -10
echo
echo "== Search for get_event and delete_event usage pattern in same method =="
rg -n 'get_event' source/ --type cpp -B2 -A15 | grep -A15 'delete_event\|delete_event'Repository: fcs-proj/FastCaloSim
Length of output: 1155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== TFCSLateralShapeParametrizationHitBase.h content (first 150 lines) =="
head -150 include/FastCaloSim/Core/TFCSLateralShapeParametrizationHitBase.h
echo
echo "== Look for test files or examples =="
find . -name "*test*" -o -name "*Test*" | grep -i "binn\|shower" | head -10
echo
echo "== Search for any delete_event invocations with grep -r =="
grep -r 'delete_event\s*(' . --include="*.cpp" --include="*.cxx" --include="*.cc" 2>/dev/null | grep -v '^Binary'Repository: fcs-proj/FastCaloSim
Length of output: 5123
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== TFCSBinnedShower::get_event implementation =="
sed -n '179,220p' source/Core/TFCSBinnedShower.cxx | cat -n
echo
echo "== TFCSBinnedShowerONNX::get_event implementation =="
sed -n '55,100p' source/Core/TFCSBinnedShowerONNX.cxx | cat -n
echo
echo "== Verify: any new[] or malloc in get_event implementations =="
rg -n 'new\s+|malloc|allocate' source/Core/TFCSBinnedShower.cxx source/Core/TFCSBinnedShowerONNX.cxx -A3 | head -40Repository: fcs-proj/FastCaloSim
Length of output: 5827
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== TFCSBinnedShower::delete_event implementation =="
sed -n '522,540p' source/Core/TFCSBinnedShower.cxx | cat -n
echo
echo "== TFCSBinnedShowerONNX::delete_event implementation =="
sed -n '507,525p' source/Core/TFCSBinnedShowerONNX.cxx | cat -nRepository: fcs-proj/FastCaloSim
Length of output: 1854
delete_event() cleanup is declared but never invoked, causing per-event memory leaks.
get_event() allocates heap memory (via new) for event data and hit storage, storing pointers in simulstate. Both TFCSBinnedShower and TFCSBinnedShowerONNX implement delete_event() to deallocate this memory, but the method is never called. The simulate() method (lines 36–87 in source/Core/TFCSBinnedShowerBase.cxx) calls get_event() at line 68 but has no matching delete_event() cleanup. This creates a per-event memory leak of the event data, hit vectors, and energy layer vectors.
Establish a cleanup mechanism that guarantees delete_event() is called once per event after all hit processing completes—either within simulate(), through a wrapper, or via RAII if simulstate lifecycle permits.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/FastCaloSim/Core/TFCSBinnedShowerBase.h` around lines 100 - 101,
simulate() calls get_event() but never invokes delete_event(), leaking per-event
heap allocations; ensure delete_event(TFCSSimulationState&) is called once per
event after all hit processing completes by adding a cleanup call in simulate()
(or wrapping simulstate in an RAII guard) so TFCSBinnedShower::delete_event and
TFCSBinnedShowerONNX::delete_event run reliably; reference the get_event /
simulate calls in TFCSBinnedShowerBase and the delete_event virtual to locate
where to insert the call or construct the RAII wrapper that invokes delete_event
in its destructor.
Collaborator
There was a problem hiding this comment.
This is an actual open problem. So far, the memory leak was not fixed as a simple delete call at the end of the function might be missed by an unexpected abort. As the leak was not critical so far, it was kept open for later fixing. Feel free to suggest a fix, if you have a solution that would be safe.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
cat -n source/Core/TFCSBinnedShowerBase.cxx
Length of output: 6946
@floern1, thanks for confirming. The challenge is indeed two-fold:
- Exception safety inside
simulate()(betweenget_event()and the nextsimulate()call) - Final-event leak (after the very last
simulate_hit(),simulate()is never called again so the lazy-cleanup at the top ofsimulate()never fires)
A plain delete_event() at the end of simulate() would actually be wrong anyway—the event data must survive until after all simulate_hit() calls complete.
Here is a two-part RAII-safe fix:
Part 1 — Exception safety inside simulate()
Use a dismissible scope guard after get_event(). On a normal return it does nothing; on an exception (or early return FCSFatal) it cleans up automatically.
// source/Core/TFCSBinnedShowerBase.cxx
FCSReturnCode TFCSBinnedShowerBase::simulate(
TFCSSimulationState& simulstate,
const TFCSTruthState* truth,
const TFCSExtrapolationState* extrapol) const
{
delete_event(simulstate); // clean up previous event (unchanged)
// ... (eta_center / phi_center / Einit setup unchanged) ...
simulstate.set_E(0);
get_event(simulstate, eta_center, phi_center, Einit, reference_layer_index);
// Dismissible scope guard: fires delete_event() unless explicitly dismissed.
// Protects against exceptions / early fatal returns after get_event().
bool event_guard_active = true;
struct EventGuard {
const TFCSBinnedShowerBase* self;
TFCSSimulationState& state;
bool& active;
~EventGuard() { if (active) self->delete_event(state); }
} event_guard{this, simulstate, event_guard_active};
for (long unsigned int layer_index = 0; layer_index < m_geo->n_layers(); ++layer_index) {
float layer_energy = get_layer_energy(simulstate, layer_index);
simulstate.set_E(layer_index, 0);
simulstate.add_E(layer_index, layer_energy);
}
if (simulstate.E() > std::numeric_limits<double>::epsilon()) {
for (int ilayer = 0; ilayer < m_geo->n_layers(); ++ilayer)
simulstate.set_Efrac(ilayer, simulstate.E(ilayer) / simulstate.E());
}
// All good: dismiss the guard so simulate_hit() can still use the event data.
event_guard_active = false;
return FCSSuccess;
}This is fully self-contained, requires no new headers, and handles any exception or FCSFatal early return between get_event() and the end of simulate().
Part 2 — Final-event cleanup via check_all_hits_simulated()
TFCSLateralShapeParametrizationHitChain already has the check_all_hits_simulated() virtual hook (recently added in this PR). Override it in TFCSBinnedShowerBase to call delete_event() once all hits are processed:
Header (include/FastCaloSim/Core/TFCSBinnedShowerBase.h):
protected:
// Called by the hit chain after each hit; when all hits are done, clean up
// event data allocated by get_event().
virtual bool check_all_hits_simulated(
Hit& hit,
TFCSSimulationState& simulstate,
const TFCSTruthState* truth,
const TFCSExtrapolationState* extrapol,
bool success) const override;Implementation (source/Core/TFCSBinnedShowerBase.cxx):
bool TFCSBinnedShowerBase::check_all_hits_simulated(
Hit& hit,
TFCSSimulationState& simulstate,
const TFCSTruthState* truth,
const TFCSExtrapolationState* extrapol,
bool success) const
{
// Delegate to base class for the standard energy-sum termination logic.
bool all_done = TFCSLateralShapeParametrizationHitChain::check_all_hits_simulated(
hit, simulstate, truth, extrapol, success);
if (all_done)
delete_event(simulstate); // safe: called exactly once, after the last hit
return all_done;
}This ensures delete_event() is called exactly once via the natural hit-chain lifecycle, even for the very last event. The safety net delete_event() at the top of the next simulate() call remains as a no-op fallback (e.g. if the hit chain skips the callback somehow).
Together these two changes cover both failure modes with no unsafe raw-pointer ownership and no changes to TFCSSimulationState's interface.
Would you like me to open a follow-up issue to track this, or create a `` to implement both parts now?
The requested changes have been implemented and a pull request has been created: View PR
This was referenced Apr 15, 2026
jbeirer
force-pushed
the
main-binnedShower
branch
from
fbded39 to
614bd1f
Compare
jbeirer
force-pushed
the
main-binnedShower
branch
from
614bd1f to
336cac7
Compare
jbeirer
force-pushed
the
main-binnedShower
branch
from
50e41ce to
27eb0c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 20
♻️ Duplicate comments (10)
source/Core/TFCSPhiModulationCorrection.cxx (3)
185-192:⚠️ Potential issue | 🟠 MajorGuard
m_geoand the returned cell before use.
m_geo->get_cell(...)(L188) will crash ifset_geometry()was never called, and there is no validity check on the returnedcellbefore callingcell.phi()(L190). This path is reachable from bothadd_phi_modulationandremove_phi_modulationduring simulation.+ if (!m_geo) { + FCS_MSG_ERROR("Geometry not set for phi modulation correction"); + return std::make_tuple(4, 0, 0); + } Position pos {}; pos.m_eta = eta; pos.m_phi = phi; const auto& cell = m_geo->get_cell(layer_index, pos); + // TODO: validate `cell` per CaloGeo's API before use. float cell_phi = cell.phi();Confirm
CaloGeo::get_cell(layer, Position)guarantees a valid reference (never a "null"/sentinel cell) for arbitrary(eta, phi, layer); if not, add an explicit invalid-cell check.#!/bin/bash ast-grep --pattern 'class CaloGeo { $$$ }' rg -nP --type=cpp -C3 '\bget_cell\s*\('🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/Core/TFCSPhiModulationCorrection.cxx` around lines 185 - 192, Guard against a missing geometry and an invalid cell before using it: in TFCSPhiModulationCorrection where you call m_geo->get_cell(layer_index, pos) (used by add_phi_modulation and remove_phi_modulation), first check that m_geo is non-null and that set_geometry() has been called, and after calling CaloGeo::get_cell validate the returned cell (or its validity flag/method) before calling cell.phi(); if either check fails, return or log an error and skip modulation for that input. Use the symbols Position, m_geo, get_cell, cell.phi(), add_phi_modulation, remove_phi_modulation, and set_geometry() to locate and implement the null/invalid-cell guards and appropriate early-exit or error logging.
294-298:⚠️ Potential issue | 🟠 Major
remove_phi_modulationis not the inverse ofadd_phi_modulation.
add_phi_modulationapplies both the modulation factor andm_energy_shift[layer][eta_index](L294-295), butremove_phi_modulation(L362) only divides by the modulation factor. As a result,remove(add(E)) != E. Additionally, L362 has no guard against a near-zero denominator (ifmodulation[phi_index] ≈ 1 − 1/m_modulation_scale, this blows up).If the asymmetry is intentional (e.g.,
energy_shiftis treated as a one-way calibration and not reverted), please document it at the declaration site.energy /= ((modulation.at(phi_index) - 1) * m_modulation_scale + 1); + const float shift = m_energy_shift.at(layer_index).at(eta_index); + if (shift > std::numeric_limits<float>::epsilon()) + energy /= shift; return energy;Also applies to: 359-364
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/Core/TFCSPhiModulationCorrection.cxx` around lines 294 - 298, The inverse mismatch: add_phi_modulation multiplies energy by ((modulation.at(phi_index) - 1) * m_modulation_scale + 1) and by m_energy_shift.at(layer_index).at(eta_index), but remove_phi_modulation only divides by the modulation factor and lacks a zero-denominator guard; update remove_phi_modulation to divide by both the same combined factor (i.e., undo both the modulation term computed from modulation.at(phi_index) and m_modulation_scale and the m_energy_shift[layer][eta]) and add a safety check to avoid dividing by values near zero (handle or return original energy / clamp / log error); if the asymmetry is intentional instead, add a clear comment at the declaration site explaining why m_energy_shift is not reverted.
114-137:⚠️ Potential issue | 🟠 MajorUnchecked ROOT file/histogram access +
std::cout+ silent override ofenergy_shiftargument.Several issues in
load_phi_modulation:
TFile::Open(L114), thehWt_Layer0lookup (L115) and its use viaGetNbinsX/GetXaxis/GetBinContent(L117, L121, L124, L127) are all unguarded. A missing file or histogram will segfault.muon_corr->Close()(L137) is also unsafe if open failed.- L134 uses
std::cout– please route through the FCS_MSG_* logger for consistency with the rest of the file.- L110 stores the caller-supplied
energy_shift, then L132-135 silently overrides it if aTParameter<double>namedenergy_shiftexists in the file. Document the intended precedence, or invert it (argument wins unless user explicitly opts into reading from file); at minimum this surprise behavior should be stated in the header.- TFile* muon_corr = TFile::Open(filename.c_str()); - TH2F* muon_corr_hist = (TH2F*)muon_corr->Get("hWt_Layer0"); - - int n_bins = muon_corr_hist->GetNbinsX(); + std::unique_ptr<TFile> muon_corr(TFile::Open(filename.c_str())); + if (!muon_corr || muon_corr->IsZombie()) { + FCS_MSG_ERROR("Failed to open phi modulation file: " << filename); + return; + } + TH2F* muon_corr_hist = dynamic_cast<TH2F*>(muon_corr->Get("hWt_Layer0")); + if (!muon_corr_hist) { + FCS_MSG_ERROR("hWt_Layer0 not found in " << filename); + return; + } + int n_bins = muon_corr_hist->GetNbinsX(); ... - TParameter<double>* param = - (TParameter<double>*)muon_corr->Get("energy_shift"); - if (param) { - energy_shifts.at(eta_index) = param->GetVal(); - std::cout << "Energy shift: " << energy_shifts.at(eta_index) << std::endl; - } - - muon_corr->Close(); + if (auto* param = + dynamic_cast<TParameter<double>*>(muon_corr->Get("energy_shift"))) { + energy_shifts.at(eta_index) = param->GetVal(); + FCS_MSG_DEBUG("Energy shift from file: " << energy_shifts.at(eta_index)); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/Core/TFCSPhiModulationCorrection.cxx` around lines 114 - 137, The load_phi_modulation code must guard ROOT accesses and stop using std::cout and surprising overrides: check the result of TFile::Open (muon_corr) and return/throw or log+exit if null; only call muon_corr->Get("hWt_Layer0") and then use GetNbinsX/GetXaxis/GetBinContent after verifying muon_corr_hist is non-null; call muon_corr->Close() only if muon_corr opened successfully; replace the std::cout line with the project logger (use FCS_MSG_INFO/FCS_MSG_WARN as appropriate) instead of printing to stdout; and change the energy_shift override logic (where you read TParameter<double> from the file and assign to energy_shifts.at(eta_index)) so that the caller-supplied energy_shifts value wins unless it is a sentinel (e.g., std::isnan(energy_shifts.at(eta_index))) or add explicit flag to opt into file overrides, and update the function/header comment to document the precedence of file vs. argument-provided energy_shift.source/Core/TFCSBinnedShowerONNX.cxx (3)
205-211:⚠️ Potential issue | 🟡 MinorAdd the missing null check before dereferencing
event.
getAuxInfo<void*>()can returnnullptr; Line 211 dereferenceseventimmediately.Proposed fix
TFCSMLCalorimeterSimulator::event_t* event = static_cast<TFCSMLCalorimeterSimulator::event_t*>( simulstate.getAuxInfo<void*>("BSEventData"_FCShash)); + if (!event) { + FCS_MSG_ERROR("No ONNX event data found in simulstate"); + return; + } float e_init = simulstate.getAuxInfo<float>("BSEinit"_FCShash);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/Core/TFCSBinnedShowerONNX.cxx` around lines 205 - 211, The code dereferences TFCSMLCalorimeterSimulator::event_t* event returned by simulstate.getAuxInfo<void*>("BSEventData"_FCShash) without checking for nullptr; add a null check after the call to simulstate.getAuxInfo and handle the error path (e.g., log an error and return/abort the function) before using event->event_data to set n_layers, ensuring subsequent code does not dereference event when it's nullptr.
462-484:⚠️ Potential issue | 🟡 MinorCheck aux-info pointers before use.
eventandhits_per_layer_ptrare both cast from aux info and dereferenced without null checks. Missing aux data will crash this path.Proposed fix
TFCSMLCalorimeterSimulator::event_t* event = static_cast<TFCSMLCalorimeterSimulator::event_t*>( simulstate.getAuxInfo<void*>("BSEventData"_FCShash)); + if (!event) { + FCS_MSG_ERROR("No ONNX event data found in simulstate"); + return std::make_tuple(0.0f, 0.0f, 0.0f); + } @@ std::vector<std::vector<long unsigned int>>* hits_per_layer_ptr = static_cast<std::vector<std::vector<long unsigned int>>*>( simulstate.getAuxInfo<void*>("BSNHits"_FCShash)); + if (!hits_per_layer_ptr) { + FCS_MSG_ERROR("No ONNX hit-count data found in simulstate"); + return std::make_tuple(0.0f, 0.0f, 0.0f); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/Core/TFCSBinnedShowerONNX.cxx` around lines 462 - 484, The code casts simulstate.getAuxInfo<void*>("BSEventData"_FCShash) and simulstate.getAuxInfo<void*>("BSNHits"_FCShash) into event (TFCSMLCalorimeterSimulator::event_t*) and hits_per_layer_ptr (std::vector<std::vector<long unsigned int>>*) and dereferences them without null checks; update the function to validate both pointers after the static_cast (e.g., if (!event) { FCS_MSG_ERROR(...); return std::make_tuple(0.0f,0.0f,0.0f); } and similarly for hits_per_layer_ptr), then proceed to call get_energy_index(layer_index, hit_index) and use hits_per_layer_ptr only when non-null to avoid crashes. Ensure log messages mention which aux key is missing ("BSEventData" or "BSNHits") and include layer_index/hit_index context.
376-405:⚠️ Potential issue | 🟡 MinorValidate
bin_indexbefore indexing sub-bin distributions.The layer dimension is checked, but
.at(bin_index)is still unguarded in both interpolation paths and the single-energy path. Invalid voxel indices will throwstd::out_of_range.Suggested guard
- if (m_sub_bin_distribution.at(e_index).size() > layer_index) { + if (m_sub_bin_distribution.at(e_index).size() > layer_index + && m_sub_bin_distribution.at(e_index).at(layer_index).size() + > static_cast<std::size_t>(bin_index)) { probabilities = m_sub_bin_distribution.at(e_index).at(layer_index).at(bin_index); }Apply the equivalent check for
e_index - 1,e_index, and theavailable_energies.size() == 1branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/Core/TFCSBinnedShowerONNX.cxx` around lines 376 - 405, The code uses .at(bin_index) without validating bin_index which can throw std::out_of_range; update the logic around m_sub_bin_distribution accesses in the interpolation branch and the single-energy branch to check that the vector for the given energy slice (m_sub_bin_distribution.at(e_index) and m_sub_bin_distribution.at(e_index - 1)) has size() > layer_index AND that their layer vectors have size() > bin_index before calling .at(bin_index); similarly guard the available_energies.size() == 1 branch by verifying m_sub_bin_distribution.at(0).at(layer_index).size() > bin_index (or handle the missing bin gracefully), and if the check fails return a safe default or emit a clear error/log rather than indexing out of range.source/Core/TFCSVoxelHistoLateralCovarianceFluctuations.cxx (3)
66-68:⚠️ Potential issue | 🟠 MajorDo not retain the
TFile-owned template histogram.Line 66 stores the
TH2*returned byGet()directly. That object is file/directory-owned, som_voxel_template[0]can become dangling after the input file closes; clone it and detach it before storing.Proposed fix
- m_voxel_template.push_back(temp); + TH2* temp_clone = static_cast<TH2*>(temp->Clone()); + temp_clone->SetDirectory(nullptr); + m_voxel_template.push_back(temp_clone);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/Core/TFCSVoxelHistoLateralCovarianceFluctuations.cxx` around lines 66 - 68, The code stores a TH2* from Get() into m_voxel_template (temp → m_voxel_template[0]) which is owned by the input TFile and may be deleted when the file closes; instead, clone the histogram and detach it from any file by calling Clone() on the TH2 returned by Get(), then SetDirectory(nullptr) (or equivalent) on the clone before pushing it into m_voxel_template so m_voxel_template stores an independent, file-detached histogram (references: temp, m_voxel_template, Get(), GetNbinsX(), GetNbinsY()).
213-229:⚠️ Potential issue | 🔴 CriticalValidate the upper
Ebinbound before indexing PCA tables.The guard still only handles empty data or
Ebin <= 0;Ebin > m_parMeans.size()will indexm_parMeans[Ebin - 1],m_transform[Ebin - 1],m_EigenValues[Ebin - 1], andm_EigenVectors[Ebin - 1]out of bounds.Proposed fix
if (m_parMeans.empty() || Ebin <= 0) { // not initialized, do nothing in case this instance is just used to clean // up memory return FCSSuccess; } + const auto ebin_index = static_cast<std::size_t>(Ebin - 1); + if (ebin_index >= m_parMeans.size() || ebin_index >= m_transform.size() + || ebin_index >= m_EigenValues.size() + || ebin_index >= m_EigenVectors.size()) { + FCS_MSG_ERROR("Invalid Ebin " << Ebin << " for loaded covariance data"); + return FCSFatal; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/Core/TFCSVoxelHistoLateralCovarianceFluctuations.cxx` around lines 213 - 229, The code only checks m_parMeans.empty() and Ebin <= 0 but must also validate Ebin does not exceed the PCA tables' size before any indexing; modify the guard in TFCSVoxelHistoLateralCovarianceFluctuations (around the block that allocates weightvec, constructs TVectorD genPars from m_parMeans[Ebin-1], and calls MultiGaus) to require 1 <= Ebin && Ebin <= m_parMeans.size() (or the minimum size among m_parMeans, m_transform, m_EigenValues, m_EigenVectors) and return FCSSuccess (or an appropriate error) if out of range, so subsequent uses of m_parMeans[Ebin-1], m_transform[Ebin-1], m_EigenValues[Ebin-1], and m_EigenVectors[Ebin-1] are safe.
236-237:⚠️ Potential issue | 🟠 MajorUse the standard normal CDF scaling.
Erf(x * 2 / pi)is not the Gaussian CDF mapping; it biases the sampled voxel weights. Usex / sqrt(2)for a standard normal draw.Proposed fix
- double cdf_val = - (TMath::Erf(genPars[count] * 2.0 / TMath::Pi()) + 1) / 2.0; + double cdf_val = + 0.5 * (1.0 + TMath::Erf(genPars[count] / TMath::Sqrt(2.0)));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/Core/TFCSVoxelHistoLateralCovarianceFluctuations.cxx` around lines 236 - 237, The current mapping uses TMath::Erf(genPars[count] * 2.0 / TMath::Pi()) which is not the standard normal CDF scaling and biases voxel weights; change the Erf argument to use x/sqrt(2) instead (i.e., use genPars[count] divided by sqrt(2) via TMath::Sqrt(2.0)) so that cdf_val = (TMath::Erf(genPars[count] / TMath::Sqrt(2.0)) + 1) / 2.0, keeping the existing genPars[count], cdf_val and TMath::Erf references.include/FastCaloSim/Core/TFCSBinnedShower.h (1)
89-91:⚠️ Potential issue | 🟠 MajorValidate hit-energy and hit-count limits.
The setters still accept
<= 0, which can cause divide-by-zero or nonsensical hit counts downstream.Proposed fix
+#include <stdexcept> + - void set_default_hit_energy(float energy) { m_default_hit_energy = energy; } + void set_default_hit_energy(float energy) + { + if (energy <= 0.F) { + throw std::invalid_argument("set_default_hit_energy requires energy > 0"); + } + m_default_hit_energy = energy; + } float get_default_hit_energy() const { return m_default_hit_energy; } - void set_max_hits_per_voxel(int max_hits) { m_max_hits_per_voxel = max_hits; } + void set_max_hits_per_voxel(int max_hits) + { + if (max_hits <= 0) { + throw std::invalid_argument( + "set_max_hits_per_voxel requires max_hits > 0"); + } + m_max_hits_per_voxel = max_hits; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/FastCaloSim/Core/TFCSBinnedShower.h` around lines 89 - 91, The setters set_default_hit_energy and set_max_hits_per_voxel currently accept non-positive values; update them to validate inputs and reject or clamp invalid values (e.g., throw std::invalid_argument or use an assert) so m_default_hit_energy > 0 and m_max_hits_per_voxel > 0 are guaranteed; specifically, in set_default_hit_energy(float energy) check energy > 0 before assigning m_default_hit_energy and in set_max_hits_per_voxel(int max_hits) check max_hits > 0 before assigning m_max_hits_per_voxel, and include a clear error message referencing the invalid value if you choose to throw.
🧹 Nitpick comments (9)
include/FastCaloSim/Core/TFCSPhiModulationCorrection.h (1)
42-54: Return getters by const reference to avoid copying large nested vectors.
get_phi_modulation(),get_min_phi(),get_min_eta(), andget_energy_shift()currently return the full nestedstd::vector<...>by value. Each call deep-copies per-layer/per-η tables, which can be sizeable and hot during simulation inspection or I/O.- std::vector<std::vector<std::vector<float>>> get_phi_modulation() const - { - return m_modulation; - }; - std::vector<std::vector<float>> get_min_eta() const { return m_min_eta; }; - std::vector<std::vector<float>> get_energy_shift() const - { - return m_energy_shift; - }; - std::vector<std::vector<std::vector<float>>> get_min_phi() const - { - return m_min_phi; - }; + const std::vector<std::vector<std::vector<float>>>& get_phi_modulation() const + { + return m_modulation; + }; + const std::vector<std::vector<float>>& get_min_eta() const { return m_min_eta; }; + const std::vector<std::vector<float>>& get_energy_shift() const + { + return m_energy_shift; + }; + const std::vector<std::vector<std::vector<float>>>& get_min_phi() const + { + return m_min_phi; + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/FastCaloSim/Core/TFCSPhiModulationCorrection.h` around lines 42 - 54, Change the four getters to return const references instead of copying the nested vectors: update get_phi_modulation(), get_min_phi(), get_min_eta(), and get_energy_shift() to return const std::vector<...>& (matching the existing member types m_modulation, m_min_phi, m_min_eta, m_energy_shift) and update their definitions accordingly so they return the member by const reference; ensure signatures remain const-qualified so callers can still use them on const objects and avoid introducing ownership or lifetime changes beyond returning a reference to the existing members.include/FastCaloSim/Core/TFCSMLCalorimeterSimulator.h (2)
69-69: Include-guard comment doesn't match the macro.Header-guard is
TFCSMLCALORIMETERSIMULATOR_Hbut the closing comment readsISF_TFCSMLCALORIMETERSIMULATOR_H. Cosmetic, but worth aligning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/FastCaloSim/Core/TFCSMLCalorimeterSimulator.h` at line 69, The include-guard closing comment mismatches the actual macro name; update the trailing comment after the `#endif` to match the guard macro TFCSMLCALORIMETERSIMULATOR_H (replace ISF_TFCSMLCALORIMETERSIMULATOR_H with TFCSMLCALORIMETERSIMULATOR_H) so the comment aligns with the guard defined at the top of TFCSMLCalorimeterSimulator.h.
6-13: Missing#include <memory>forstd::unique_ptr.
std::unique_ptr<VNetworkBase>is used on line 54 but<memory>is not included here; it's only brought in transitively via other headers. Add the explicit include to avoid breakage if upstream headers stop transitively exposing it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/FastCaloSim/Core/TFCSMLCalorimeterSimulator.h` around lines 6 - 13, The header is missing an explicit `#include` <memory> required for std::unique_ptr<VNetworkBase> (used in TFCSMLCalorimeterSimulator), so add `#include` <memory> near the other standard includes at the top of the file to avoid relying on transitive includes; update TFCSMLCalorimeterSimulator's header includes (before using std::unique_ptr<VNetworkBase>) to include <memory>.source/Core/TFCSBinnedShower.cxx (3)
797-800: Use FCS logging macros instead ofstd::cerr/std::cout.
load_sub_bin_distributionmixesstd::cerr(line 798) andstd::cout(lines 852-857) withFCS_MSG_*used elsewhere in this class. This bypasses the FCS log-level plumbing and pollutes stdout in production jobs. UseFCS_MSG_ERROR/FCS_MSG_INFO/FCS_MSG_DEBUGfor consistency; the loop at 851-859 looks like debugging leftover and can be behindFCS_MSG_DEBUG.Also applies to: 852-858
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/Core/TFCSBinnedShower.cxx` around lines 797 - 800, In load_sub_bin_distribution replace direct std::cerr/std::cout usage with the FCS logging macros used elsewhere: use FCS_MSG_ERROR for the file-open failure message (replace the std::cerr block that checks file->IsZombie()), and convert the informational/loop prints (the std::cout block around the loop at lines ~851-859) to FCS_MSG_DEBUG (or FCS_MSG_INFO if you prefer higher visibility) so they respect FCS log levels; update the message text to be descriptive and keep the loop printing guarded by the debug/info macro.
471-474:R_max = R_min;afterR_minwas just mutated collapses the range to zero width.Lines 471-472 set
R_min = R_min + r/2*(R_max - R_min); R_max = R_min;. Downstream,get_coordinateschecksTMath::Abs(R_max - R_min) > epsilonand — because they are now equal — usesR = R_minwithout sampling. That appears intentional (collapsing to a point after inverse-CDF sampling), but the ordering is subtle: line 472 uses the newR_min, not the original midpoint. Please add a comment confirming this is the intended single-point assignment, or precompute the target before reassigning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/Core/TFCSBinnedShower.cxx` around lines 471 - 474, The assignment collapses the radial range to a single point by first mutating R_min then setting R_max = R_min, which can be confusing because R_max gets the new R_min rather than the original midpoint; update TFCSBinnedShower.cxx to either precompute the intended target (e.g., compute a local R_target = R_min + r/2*(R_max - R_min) and then set R_min = R_max = R_target) or add an explicit comment immediately above these lines stating that collapsing to that single-point value is intentional and that R_max is meant to equal the newly computed R_min so downstream code samples the fixed radius.
640-663:bin_indexisfloat— causes FP↔integer comparisons and risks precision loss for large dimensions.
bin_indexis declaredfloaton line 645 but used as a cumulative counter and compared againstdims.at(1) - 1(which ishsize_t). For modest bin counts this works, but using float for integer indexing is a code smell and may silently truncate at very largedims.at(1)(> 2^24). Use an integral type.♻️ Proposed fix
- long unsigned int event_index = i / dims.at(1); - float bin_index = i % dims.at(1); + long unsigned int event_index = i / dims.at(1); + hsize_t bin_index = i % dims.at(1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/Core/TFCSBinnedShower.cxx` around lines 640 - 663, The local variable bin_index should be an integral type instead of float to avoid FP↔integer comparisons and precision loss; replace the float bin_index in the loop with an integer/hsize_t (e.g. hsize_t or size_t) computed as i % dims.at(1), and ensure compatibility with bin_index_vector (either change bin_index_vector's element type from unsigned int to the chosen integer type or static_cast when pushing) and keep the comparison against dims.at(1) - 1 using the same integral type; update usages in the loop and the call to set_layer_energy (which takes bin_index_vector) accordingly.CMakeLists.txt (1)
51-51: MPI should likely beREQUIREDto fail fast with a clear message.
find_package(MPI COMPONENTS C CXX)is called withoutREQUIRED. Per the comment above, HDF5 depends on MPI being resolved first. If MPI is not found here, the subsequentfind_package(HDF5 REQUIRED ...)will likely fail with a confusing error about HDF5 transitive deps rather than pointing at the missing MPI. Consider making MPIREQUIREDor guarding with a descriptiveFATAL_ERRORmirroring the Boost block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` at line 51, The find_package(MPI COMPONENTS C CXX) call should fail fast and provide a clear message if MPI is missing; update the CMakeLists to either make the call required (add REQUIRED to find_package(MPI COMPONENTS C CXX)) or add an explicit check after find_package that calls message(FATAL_ERROR ...) with a descriptive error like "MPI required but not found" (similar to the Boost handling) before calling find_package(HDF5 REQUIRED ...) so HDF5 errors don't mask a missing MPI.source/Core/TFCSBinnedShowerBase.cxx (1)
69-83: Signedness mismatch between the two layer loops.Line 69 iterates with
long unsigned int layer_index, while line 80 iterates withint ilayeragainstm_geo->n_layers()(unsigned). This produces a signed/unsigned comparison warning and may truncate for large layer counts. Prefer a consistentlong unsigned int(orsize_t) in both loops.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/Core/TFCSBinnedShowerBase.cxx` around lines 69 - 83, The two loops over m_geo->n_layers() use different index types causing signed/unsigned warnings; update both loops to use a consistent unsigned index type (prefer size_t or long unsigned int) — e.g. change the first loop's `long unsigned int layer_index` or the second loop's `int ilayer` so both are `size_t ilayer` (or `long unsigned int ilayer`), and then use that single symbol (`ilayer` or `layer_index`) when calling get_layer_energy(simulstate, ...), simulstate.set_E(...), simulstate.add_E(...), simulstate.E(...), and simulstate.set_Efrac(...) to avoid signed/unsigned comparisons and potential truncation.source/Core/TFCSLateralShapeParametrizationHitChain.cxx (1)
263-267:nhit_signedcan be 0 or negative whenElayer/Ehitrounds down.
TMath::Nint(Elayer/Ehit)rounds to nearest; for small ratios this yields 0 (handled by the< 1clamp). However,ElayerandEhitcan both be negative in this framework (the code explicitly tolerates same-sign negatives viaEhit * Elayer <= 0at line 253). A negative ratio rounds to a negative integer; the clampif (nhit_signed < 1) nhit_signed = 1;handles this, but the subsequent loop withsumEhit += hit.E()and comparison|sumEhit| >= |Elayer|should terminate anyway. Worth verifying with a negative-energy test.The
TODO: should read simulate value!comment on line 263 also suggests the nhit derivation doesn't reuse the previously stored"FCSHitChainNHits"aux-info. Consider reading it here to avoid recomputation divergence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/Core/TFCSLateralShapeParametrizationHitChain.cxx` around lines 263 - 267, Compute nhit from the stored aux "FCSHitChainNHits" when available instead of recomputing; if absent, derive nhit using the magnitudes to avoid negative values by doing nhit_signed = TMath::Nint(std::fabs(Elayer) / std::fabs(Ehit)), then clamp nhit_signed >= 1 and cast to unsigned nhit as before (use symbols nhit_signed, nhit, Elayer, Ehit, and the aux key "FCSHitChainNHits" in TFCSLateralShapeParametrizationHitChain.cxx); this preserves the existing loop logic that compares absolute energies and avoids negative nhit artifacts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CMakeLists.txt`:
- Around line 48-56: The CMake block uses the imported target HDF5::HDF5 which
requires CMake >= 3.19 and also has a redundant HDF5_FOUND check after
find_package(REQUIRED); update CMakeLists by either raising
cmake_minimum_required(VERSION 3.19) (so HDF5::HDF5 is valid) or keep the
existing minimum and switch usages of HDF5::HDF5 to the older variables
${HDF5_LIBRARIES} and ${HDF5_INCLUDE_DIRS} (update any target_link_libraries or
include_directories that reference HDF5::HDF5), and remove the dead if(NOT
HDF5_FOUND) message(FATAL_ERROR ...) block since find_package(HDF5 REQUIRED ...)
already aborts on failure.
In `@include/FastCaloSim/Core/TFCSBinnedShower.h`:
- Around line 6-7: The header TFCSBinnedShower.h uses std::string in
declarations (e.g., in the TFCSBinnedShower class method parameters and a member
variable) but doesn't include <string>, relying on transitive includes; add
`#include` <string> at the top of this header so the file is self-contained and
all declarations using std::string (methods in TFCSBinnedShower and its
string-typed member) compile without depending on other headers.
In `@include/FastCaloSim/Core/TFCSBinnedShowerONNX.h`:
- Around line 52-56: load_simulator currently overwrites the raw pointer
m_ai_simulator with a new TFCSMLCalorimeterSimulator instance without deleting
the previous one, causing a memory leak; either delete the existing instance
before assigning a new one (e.g., check if m_ai_simulator and delete it) or
better convert m_ai_simulator to std::unique_ptr<TFCSMLCalorimeterSimulator> and
replace the new allocation with std::make_unique, and when switching to
unique_ptr ensure copy ctor/assignment are deleted or use defaulted move
semantics to prevent accidental copies; apply the same fix for the other
occurrence referenced near lines 178-179.
- Around line 6-7: The header TFCSBinnedShowerONNX.h is not self-contained
because its public API uses std::string in several method parameters (e.g.
methods declared on class TFCSBinnedShowerONNX) but it doesn't include <string>;
add `#include` <string> alongside the existing `#include` <tuple> and `#include`
<vector> so the header compiles standalone and all declarations using
std::string (the public methods referenced in the review) are valid.
- Around line 46-49: Reject invalid values in the setters: in
set_default_hit_energy(float) verify energy > 0 and in
set_max_hits_per_voxel(int) verify max_hits > 0; if invalid, throw a
std::invalid_argument with a descriptive message (include the parameter name and
value) so callers cannot set m_default_hit_energy or m_max_hits_per_voxel to
values that would break compute_n_hits_and_elayer(); update any tests or
callsites accordingly.
In `@include/FastCaloSim/Core/TFCSMLCalorimeterSimulator.h`:
- Around line 44-51: The setInputShapes method currently calls
layer_boundaries.back() and assumes sizes match which is UB when
layer_boundaries is empty; update setInputShapes to validate inputs: check that
layer_boundaries is not empty and that used_layers.size() and
layer_boundaries.size() meet the expected relationship (e.g.
layer_boundaries.size() == used_layers.size() + 1 or other required invariant),
and if the check fails return/throw a clear std::invalid_argument (or log and
return) instead of dereferencing back(); then safely assign m_layer_boundaries,
m_used_layers and set m_nVoxels and m_nLayers from validated values (m_nVoxels =
layer_boundaries.back(), m_nLayers = used_layers.size()) so the code never
invokes back() on an empty vector.
In `@include/FastCaloSim/Core/TFCSPhiModulationCorrection.h`:
- Around line 63-72: get_phi_cell_size currently checks the signed eta so
negative endcap hits get mis-segmented; update get_phi_cell_size (or its caller
get_eta_and_phi_index) to use the absolute pseudorapidity (e.g. fabs(eta) or an
eta_abs parameter) when deciding between 1024/768/256 segmentation so hits with
|η|>2.5 on the negative side return 2π/256; also replace the literal
1024/768/256 numbers with named constants (e.g. PHI_SEG_BARREL,
PHI_SEG_ENDCAP_MID, PHI_SEG_ENDCAP_FINE) to remove magic numbers and make future
geometry changes easier to maintain.
In `@source/Core/TFCSBinnedShower.cxx`:
- Around line 383-418: The code assumes m_sub_bin_distribution has entries for
every energy/layer/bin combination and directly calls .at(bin_index) which can
throw if that bin is missing; update the logic to either (A) enforce/fix
consistency when loading by modifying load_sub_bin_distribution() to ensure
every energy index contains vectors sized for all layers and bins (filling
missing bin entries with a default zeroed vector and/or failing loudly with a
clear error), or (B) add defensive checks where m_sub_bin_distribution is read
(in the block using available_energies, e_index, layer_index, bin_index) to
verify m_sub_bin_distribution.at(e_index).at(layer_index).size() > bin_index
before calling .at(bin_index) and handle the missing-bin case (e.g. skip, use
zeros, or log+error) so no std::out_of_range is thrown.
- Around line 268-281: get_n_hits currently calls
hits_per_layer_ptr->at(layer_index).back() which is undefined when the inner
vector is empty; update TFCSBinnedShower::get_n_hits to first validate
layer_index is within hits_per_layer_ptr->size() and then check if
hits_per_layer_ptr->at(layer_index).empty() and return 0 in that case, otherwise
return .back(); reference hits_per_layer_ptr and ensure the logic is consistent
with how compute_n_hits_and_elayer populates the inner vectors.
In `@source/Core/TFCSBinnedShowerBase.cxx`:
- Around line 44-53: The code currently switches to endcap layer based only on
positive eta; change the condition to use the absolute eta so both positive and
negative endcaps pick EME2: after computing eta_center from
extrapol->eta(reference_layer_index, Cell::SubPos::MID), replace the check if
(eta_center > 1.4) with if (std::abs(eta_center) > 1.4) (or fabs) and then set
reference_layer_index = 6; keep the subsequent recomputation of eta_center and
phi_center from extrapol using the (possibly updated) reference_layer_index and
add an `#include` <cmath> if needed.
In `@source/Core/TFCSBinnedShowerONNX.cxx`:
- Around line 63-67: The aux-info key "BSEventData"_FCShash is shared with the
non-ONNX binned shower but stores a different type here
(TFCSMLCalorimeterSimulator::event_t), so change to an ONNX-specific key (e.g.
"BSEventDataONNX"_FCShash) wherever you set, get, or delete this aux info:
update the setter in TFCSBinnedShowerONNX::compute_n_hits_and_elayer() (the
simulstate.setAuxInfo<void*>("BSEventData"... call that currently stores new
TFCSMLCalorimeterSimulator::event_t), the accessor in
TFCSBinnedShowerONNX::get_hit_position_and_energy() (simulstate.getAuxInfo
retrieval/cast), and the cleanup in TFCSBinnedShowerONNX::delete_event()
(simulstate.erase/get+delete sequence) to use the new unique key consistently so
types cannot be mixed.
- Around line 249-262: The method TFCSBinnedShowerONNX::get_n_hits currently
calls hits_per_layer_ptr->at(layer_index).back() without validating that
layer_index exists or that the layer's vector is non-empty; update the function
to first check that hits_per_layer_ptr is non-null (already done), then ensure
layer_index < hits_per_layer_ptr->size() and that
hits_per_layer_ptr->at(layer_index).empty() is false before calling .back(); if
either check fails return 0 so the base class can skip hit simulation cleanly.
Use the existing symbols hits_per_layer_ptr, layer_index and
TFCSBinnedShowerONNX::get_n_hits to locate and modify the code.
- Around line 60-61: Before calling m_ai_simulator->getEvent(...), add a
null-check for m_ai_simulator (i.e. guard that load_simulator() was called) and
return early with an error status/log if it is null; update the code around the
TFCSMLCalorimeterSimulator::event_t& event =
m_ai_simulator->getEvent(simulstate, eta_center, e_init); site to check if
(m_ai_simulator == nullptr) { log an error and return an appropriate
failure/early-exit value } so getEvent() is never invoked on a null pointer.
- Around line 3-5: TFCSBinnedShowerONNX.cxx currently relies on transitive
includes for several standard facilities; add the missing direct includes to fix
toolchain-dependency issues: include <algorithm> for std::upper_bound, std::min,
std::max; <limits> for std::numeric_limits; <map> for std::map; and <iostream>
for std::cout and std::cerr so the symbols used in TFCSBinnedShowerONNX.cxx are
declared explicitly.
In `@source/Core/TFCSLateralShapeParametrizationFixedHitChain.cxx`:
- Around line 52-82: The branch that runs when current_hit >= nhit blindly
zeroes the layer energy when nhit == 0; fix by short-circuiting early for nhit
== 0 (treat it like the nhit_signed < 0 case) so you return true / skip all
energy updates when no hits were expected, or alternatively preserve the
existing simulstate.E(calosample()) by skipping the block that sets
FCSFixedHitChainEnergySum and set_E when nhit==0; modify the logic around the
nhit/current_hit check in TFCSLateralShapeParametrizationFixedHitChain (the code
using nhit, current_hit, simulstate, "FCSFixedHitChainEnergySum"_FCShash,
setAuxInfo, set_E, and calosample()) to implement the early-return or
conditional guard so pre-existing layer energy is not overwritten when nhit is
zero.
In `@source/Core/TFCSLateralShapeParametrizationHitChain.cxx`:
- Around line 384-394: The FCSHitChainEnergySum aux-info stored on simulstate is
never cleared and can leak between chains; modify
TFCSLateralShapeParametrizationHitChain to reset or erase
"FCSHitChainEnergySum"_FCShash on entry to simulate() (and/or on successful exit
from check_all_hits_simulated()) so stale values don't affect the
sum/termination logic — locate the places using
simulstate.getAuxInfo<float>("FCSHitChainEnergySum"_FCShash),
simulstate.setAuxInfo<float>("FCSHitChainEnergySum"_FCShash, ...), and hit.idx()
to add a reset (set to 0 or remove the aux-info) before accumulating and after
finishing the chain.
In `@source/Core/TFCSPhiModulationCorrection.cxx`:
- Around line 170-183: The eta-range check using
m_min_eta.at(layer_index).at(eta_index) and
m_min_eta.at(layer_index).at(eta_index + 1) is unreachable after using
std::upper_bound to compute eta_index; remove this dead block from
TFCSPhiModulationCorrection.cxx (the error-logging + return tuple) or replace it
with an explicit validity check: add and consult a stored per-bin flag (e.g.,
m_eta_bin_valid[layer_index][eta_index]) to reject gaps if that was the intent.
Update references to eta_index and m_min_eta accordingly so the code either
drops the redundant bounds test or checks the new validity flag before returning
the error tuple.
- Around line 143-153: The public method
TFCSPhiModulationCorrection::get_eta_and_phi_index currently calls
m_min_eta.at(layer_index) without bounds checking; add an early guard that if
layer_index >= m_min_eta.size() (compare using the same unsigned type) returns a
sentinel error tuple (e.g. std::make_tuple(-1, 0ul, 0ul)), and then use
operator[] (or at() safely) for subsequent accesses; ensure callers can detect
the error via the negative int return and update any callers if needed.
In `@source/Core/TFCSVoxelHistoLateralCovarianceFluctuations.cxx`:
- Around line 78-96: The dynamic_cast results for the objects retrieved from
inputfile (the TVectorT<double>* used to fill parMeans and the
TMatrixTSym<double>* used to initialize covMatrix) need null checks before
dereferencing: after obtaining obj for ("parMeans" + label) perform auto* vec =
dynamic_cast<TVectorT<double>*>(obj) and if (!vec) log an FCS_MSG_ERROR
including the label and return false, then assign parMeans = *vec and push to
m_parMeans; similarly after obtaining obj for ("covMatrix" + label) perform
auto* mat = dynamic_cast<TMatrixTSym<double>*>(obj) and if (!mat) log an
FCS_MSG_ERROR including the label and return false, then assign covMatrix =
*mat. Ensure the debug printing still uses obj->Print() only when the cast
succeeded (or use vec/mat for printing) and keep existing ResizeTo behavior for
parMeans.
---
Duplicate comments:
In `@include/FastCaloSim/Core/TFCSBinnedShower.h`:
- Around line 89-91: The setters set_default_hit_energy and
set_max_hits_per_voxel currently accept non-positive values; update them to
validate inputs and reject or clamp invalid values (e.g., throw
std::invalid_argument or use an assert) so m_default_hit_energy > 0 and
m_max_hits_per_voxel > 0 are guaranteed; specifically, in
set_default_hit_energy(float energy) check energy > 0 before assigning
m_default_hit_energy and in set_max_hits_per_voxel(int max_hits) check max_hits
> 0 before assigning m_max_hits_per_voxel, and include a clear error message
referencing the invalid value if you choose to throw.
In `@source/Core/TFCSBinnedShowerONNX.cxx`:
- Around line 205-211: The code dereferences
TFCSMLCalorimeterSimulator::event_t* event returned by
simulstate.getAuxInfo<void*>("BSEventData"_FCShash) without checking for
nullptr; add a null check after the call to simulstate.getAuxInfo and handle the
error path (e.g., log an error and return/abort the function) before using
event->event_data to set n_layers, ensuring subsequent code does not dereference
event when it's nullptr.
- Around line 462-484: The code casts
simulstate.getAuxInfo<void*>("BSEventData"_FCShash) and
simulstate.getAuxInfo<void*>("BSNHits"_FCShash) into event
(TFCSMLCalorimeterSimulator::event_t*) and hits_per_layer_ptr
(std::vector<std::vector<long unsigned int>>*) and dereferences them without
null checks; update the function to validate both pointers after the static_cast
(e.g., if (!event) { FCS_MSG_ERROR(...); return std::make_tuple(0.0f,0.0f,0.0f);
} and similarly for hits_per_layer_ptr), then proceed to call
get_energy_index(layer_index, hit_index) and use hits_per_layer_ptr only when
non-null to avoid crashes. Ensure log messages mention which aux key is missing
("BSEventData" or "BSNHits") and include layer_index/hit_index context.
- Around line 376-405: The code uses .at(bin_index) without validating bin_index
which can throw std::out_of_range; update the logic around
m_sub_bin_distribution accesses in the interpolation branch and the
single-energy branch to check that the vector for the given energy slice
(m_sub_bin_distribution.at(e_index) and m_sub_bin_distribution.at(e_index - 1))
has size() > layer_index AND that their layer vectors have size() > bin_index
before calling .at(bin_index); similarly guard the available_energies.size() ==
1 branch by verifying m_sub_bin_distribution.at(0).at(layer_index).size() >
bin_index (or handle the missing bin gracefully), and if the check fails return
a safe default or emit a clear error/log rather than indexing out of range.
In `@source/Core/TFCSPhiModulationCorrection.cxx`:
- Around line 185-192: Guard against a missing geometry and an invalid cell
before using it: in TFCSPhiModulationCorrection where you call
m_geo->get_cell(layer_index, pos) (used by add_phi_modulation and
remove_phi_modulation), first check that m_geo is non-null and that
set_geometry() has been called, and after calling CaloGeo::get_cell validate the
returned cell (or its validity flag/method) before calling cell.phi(); if either
check fails, return or log an error and skip modulation for that input. Use the
symbols Position, m_geo, get_cell, cell.phi(), add_phi_modulation,
remove_phi_modulation, and set_geometry() to locate and implement the
null/invalid-cell guards and appropriate early-exit or error logging.
- Around line 294-298: The inverse mismatch: add_phi_modulation multiplies
energy by ((modulation.at(phi_index) - 1) * m_modulation_scale + 1) and by
m_energy_shift.at(layer_index).at(eta_index), but remove_phi_modulation only
divides by the modulation factor and lacks a zero-denominator guard; update
remove_phi_modulation to divide by both the same combined factor (i.e., undo
both the modulation term computed from modulation.at(phi_index) and
m_modulation_scale and the m_energy_shift[layer][eta]) and add a safety check to
avoid dividing by values near zero (handle or return original energy / clamp /
log error); if the asymmetry is intentional instead, add a clear comment at the
declaration site explaining why m_energy_shift is not reverted.
- Around line 114-137: The load_phi_modulation code must guard ROOT accesses and
stop using std::cout and surprising overrides: check the result of TFile::Open
(muon_corr) and return/throw or log+exit if null; only call
muon_corr->Get("hWt_Layer0") and then use GetNbinsX/GetXaxis/GetBinContent after
verifying muon_corr_hist is non-null; call muon_corr->Close() only if muon_corr
opened successfully; replace the std::cout line with the project logger (use
FCS_MSG_INFO/FCS_MSG_WARN as appropriate) instead of printing to stdout; and
change the energy_shift override logic (where you read TParameter<double> from
the file and assign to energy_shifts.at(eta_index)) so that the caller-supplied
energy_shifts value wins unless it is a sentinel (e.g.,
std::isnan(energy_shifts.at(eta_index))) or add explicit flag to opt into file
overrides, and update the function/header comment to document the precedence of
file vs. argument-provided energy_shift.
In `@source/Core/TFCSVoxelHistoLateralCovarianceFluctuations.cxx`:
- Around line 66-68: The code stores a TH2* from Get() into m_voxel_template
(temp → m_voxel_template[0]) which is owned by the input TFile and may be
deleted when the file closes; instead, clone the histogram and detach it from
any file by calling Clone() on the TH2 returned by Get(), then
SetDirectory(nullptr) (or equivalent) on the clone before pushing it into
m_voxel_template so m_voxel_template stores an independent, file-detached
histogram (references: temp, m_voxel_template, Get(), GetNbinsX(), GetNbinsY()).
- Around line 213-229: The code only checks m_parMeans.empty() and Ebin <= 0 but
must also validate Ebin does not exceed the PCA tables' size before any
indexing; modify the guard in TFCSVoxelHistoLateralCovarianceFluctuations
(around the block that allocates weightvec, constructs TVectorD genPars from
m_parMeans[Ebin-1], and calls MultiGaus) to require 1 <= Ebin && Ebin <=
m_parMeans.size() (or the minimum size among m_parMeans, m_transform,
m_EigenValues, m_EigenVectors) and return FCSSuccess (or an appropriate error)
if out of range, so subsequent uses of m_parMeans[Ebin-1], m_transform[Ebin-1],
m_EigenValues[Ebin-1], and m_EigenVectors[Ebin-1] are safe.
- Around line 236-237: The current mapping uses TMath::Erf(genPars[count] * 2.0
/ TMath::Pi()) which is not the standard normal CDF scaling and biases voxel
weights; change the Erf argument to use x/sqrt(2) instead (i.e., use
genPars[count] divided by sqrt(2) via TMath::Sqrt(2.0)) so that cdf_val =
(TMath::Erf(genPars[count] / TMath::Sqrt(2.0)) + 1) / 2.0, keeping the existing
genPars[count], cdf_val and TMath::Erf references.
---
Nitpick comments:
In `@CMakeLists.txt`:
- Line 51: The find_package(MPI COMPONENTS C CXX) call should fail fast and
provide a clear message if MPI is missing; update the CMakeLists to either make
the call required (add REQUIRED to find_package(MPI COMPONENTS C CXX)) or add an
explicit check after find_package that calls message(FATAL_ERROR ...) with a
descriptive error like "MPI required but not found" (similar to the Boost
handling) before calling find_package(HDF5 REQUIRED ...) so HDF5 errors don't
mask a missing MPI.
In `@include/FastCaloSim/Core/TFCSMLCalorimeterSimulator.h`:
- Line 69: The include-guard closing comment mismatches the actual macro name;
update the trailing comment after the `#endif` to match the guard macro
TFCSMLCALORIMETERSIMULATOR_H (replace ISF_TFCSMLCALORIMETERSIMULATOR_H with
TFCSMLCALORIMETERSIMULATOR_H) so the comment aligns with the guard defined at
the top of TFCSMLCalorimeterSimulator.h.
- Around line 6-13: The header is missing an explicit `#include` <memory> required
for std::unique_ptr<VNetworkBase> (used in TFCSMLCalorimeterSimulator), so add
`#include` <memory> near the other standard includes at the top of the file to
avoid relying on transitive includes; update TFCSMLCalorimeterSimulator's header
includes (before using std::unique_ptr<VNetworkBase>) to include <memory>.
In `@include/FastCaloSim/Core/TFCSPhiModulationCorrection.h`:
- Around line 42-54: Change the four getters to return const references instead
of copying the nested vectors: update get_phi_modulation(), get_min_phi(),
get_min_eta(), and get_energy_shift() to return const std::vector<...>&
(matching the existing member types m_modulation, m_min_phi, m_min_eta,
m_energy_shift) and update their definitions accordingly so they return the
member by const reference; ensure signatures remain const-qualified so callers
can still use them on const objects and avoid introducing ownership or lifetime
changes beyond returning a reference to the existing members.
In `@source/Core/TFCSBinnedShower.cxx`:
- Around line 797-800: In load_sub_bin_distribution replace direct
std::cerr/std::cout usage with the FCS logging macros used elsewhere: use
FCS_MSG_ERROR for the file-open failure message (replace the std::cerr block
that checks file->IsZombie()), and convert the informational/loop prints (the
std::cout block around the loop at lines ~851-859) to FCS_MSG_DEBUG (or
FCS_MSG_INFO if you prefer higher visibility) so they respect FCS log levels;
update the message text to be descriptive and keep the loop printing guarded by
the debug/info macro.
- Around line 471-474: The assignment collapses the radial range to a single
point by first mutating R_min then setting R_max = R_min, which can be confusing
because R_max gets the new R_min rather than the original midpoint; update
TFCSBinnedShower.cxx to either precompute the intended target (e.g., compute a
local R_target = R_min + r/2*(R_max - R_min) and then set R_min = R_max =
R_target) or add an explicit comment immediately above these lines stating that
collapsing to that single-point value is intentional and that R_max is meant to
equal the newly computed R_min so downstream code samples the fixed radius.
- Around line 640-663: The local variable bin_index should be an integral type
instead of float to avoid FP↔integer comparisons and precision loss; replace the
float bin_index in the loop with an integer/hsize_t (e.g. hsize_t or size_t)
computed as i % dims.at(1), and ensure compatibility with bin_index_vector
(either change bin_index_vector's element type from unsigned int to the chosen
integer type or static_cast when pushing) and keep the comparison against
dims.at(1) - 1 using the same integral type; update usages in the loop and the
call to set_layer_energy (which takes bin_index_vector) accordingly.
In `@source/Core/TFCSBinnedShowerBase.cxx`:
- Around line 69-83: The two loops over m_geo->n_layers() use different index
types causing signed/unsigned warnings; update both loops to use a consistent
unsigned index type (prefer size_t or long unsigned int) — e.g. change the first
loop's `long unsigned int layer_index` or the second loop's `int ilayer` so both
are `size_t ilayer` (or `long unsigned int ilayer`), and then use that single
symbol (`ilayer` or `layer_index`) when calling get_layer_energy(simulstate,
...), simulstate.set_E(...), simulstate.add_E(...), simulstate.E(...), and
simulstate.set_Efrac(...) to avoid signed/unsigned comparisons and potential
truncation.
In `@source/Core/TFCSLateralShapeParametrizationHitChain.cxx`:
- Around line 263-267: Compute nhit from the stored aux "FCSHitChainNHits" when
available instead of recomputing; if absent, derive nhit using the magnitudes to
avoid negative values by doing nhit_signed = TMath::Nint(std::fabs(Elayer) /
std::fabs(Ehit)), then clamp nhit_signed >= 1 and cast to unsigned nhit as
before (use symbols nhit_signed, nhit, Elayer, Ehit, and the aux key
"FCSHitChainNHits" in TFCSLateralShapeParametrizationHitChain.cxx); this
preserves the existing loop logic that compares absolute energies and avoids
negative nhit artifacts.
🪄 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: 77bf2795-b040-43a2-acd9-9bef032efb39
📒 Files selected for processing (19)
CMakeLists.txtinclude/FastCaloSim/Core/TFCSBinnedShower.hinclude/FastCaloSim/Core/TFCSBinnedShowerBase.hinclude/FastCaloSim/Core/TFCSBinnedShowerONNX.hinclude/FastCaloSim/Core/TFCSLateralShapeParametrizationFixedHitChain.hinclude/FastCaloSim/Core/TFCSLateralShapeParametrizationHitBase.hinclude/FastCaloSim/Core/TFCSLateralShapeParametrizationHitChain.hinclude/FastCaloSim/Core/TFCSMLCalorimeterSimulator.hinclude/FastCaloSim/Core/TFCSPhiModulationCorrection.hinclude/FastCaloSim/Core/TFCSVoxelHistoLateralCovarianceFluctuations.hinclude/FastCaloSim/LinkDef.hsource/Core/TFCSBinnedShower.cxxsource/Core/TFCSBinnedShowerBase.cxxsource/Core/TFCSBinnedShowerONNX.cxxsource/Core/TFCSLateralShapeParametrizationFixedHitChain.cxxsource/Core/TFCSLateralShapeParametrizationHitChain.cxxsource/Core/TFCSMLCalorimeterSimulator.cxxsource/Core/TFCSPhiModulationCorrection.cxxsource/Core/TFCSVoxelHistoLateralCovarianceFluctuations.cxx
✅ Files skipped from review due to trivial changes (1)
- source/Core/TFCSMLCalorimeterSimulator.cxx
🚧 Files skipped from review as they are similar to previous changes (4)
- include/FastCaloSim/Core/TFCSLateralShapeParametrizationHitBase.h
- include/FastCaloSim/Core/TFCSLateralShapeParametrizationFixedHitChain.h
- include/FastCaloSim/LinkDef.h
- include/FastCaloSim/Core/TFCSVoxelHistoLateralCovarianceFluctuations.h
Comment on lines
+384
to
+394
| float sumEhit = 0; | ||
| if (simulstate.hasAuxInfo("FCSHitChainEnergySum"_FCShash) && hit.idx() > 1) { | ||
| sumEhit = simulstate.getAuxInfo<float>("FCSHitChainEnergySum"_FCShash); | ||
| } | ||
|
|
||
| sumEhit += hit.E(); | ||
|
|
||
| simulstate.setAuxInfo<float>("FCSHitChainEnergySum"_FCShash, sumEhit); | ||
|
|
||
| float Elayer = simulstate.E(calosample()); | ||
| return (std::abs(sumEhit) >= std::abs(Elayer)); |
There was a problem hiding this comment.
FCSHitChainEnergySum aux-info is never cleared; potentially reused across unrelated chains.
The accumulator is keyed by a single fixed hash and never reset when simulate() exits. The hit.idx() > 1 guard makes the first two hits of each new simulate() call ignore any stale value, so the steady-state is self-correcting as long as simulstate is used by one chain at a time. However, if two TFCSLateralShapeParametrizationHitChain instances share the same simulstate or one is nested inside another, the stored FCSHitChainEnergySum from one chain can leak into the termination condition of the other.
Consider resetting the aux-info to 0 (or erasing it) on entry to simulate() and/or on successful exit from check_all_hits_simulated().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/Core/TFCSLateralShapeParametrizationHitChain.cxx` around lines 384 -
394, The FCSHitChainEnergySum aux-info stored on simulstate is never cleared and
can leak between chains; modify TFCSLateralShapeParametrizationHitChain to reset
or erase "FCSHitChainEnergySum"_FCShash on entry to simulate() (and/or on
successful exit from check_all_hits_simulated()) so stale values don't affect
the sum/termination logic — locate the places using
simulstate.getAuxInfo<float>("FCSHitChainEnergySum"_FCShash),
simulstate.setAuxInfo<float>("FCSHitChainEnergySum"_FCShash, ...), and hit.idx()
to add a reset (set to 0 or remove the aux-info) before accumulating and after
finishing the chain.
Comment on lines
+143
to
+153
| std::tuple<int, long unsigned int, long unsigned int> | ||
| TFCSPhiModulationCorrection::get_eta_and_phi_index( | ||
| float phi, float eta, long unsigned int layer_index) const | ||
| { | ||
| float eta_abs = TMath::Abs(eta); | ||
|
|
||
| auto eta_it = std::upper_bound(m_min_eta.at(layer_index).begin(), | ||
| m_min_eta.at(layer_index).end(), | ||
| eta_abs); | ||
| long unsigned int eta_index = | ||
| std::distance(m_min_eta.at(layer_index).begin(), eta_it) - 1; |
There was a problem hiding this comment.
Public method throws on out-of-range layer_index.
get_eta_and_phi_index is declared public but unconditionally does m_min_eta.at(layer_index) (L149) before any bounds check. The add_/remove_phi_modulation overloads guard against this in the callers, but an external user (or a future caller) invoking this directly will get a std::out_of_range. Consider adding the same layer_index >= m_min_eta.size() early-out here and returning an error tuple.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/Core/TFCSPhiModulationCorrection.cxx` around lines 143 - 153, The
public method TFCSPhiModulationCorrection::get_eta_and_phi_index currently calls
m_min_eta.at(layer_index) without bounds checking; add an early guard that if
layer_index >= m_min_eta.size() (compare using the same unsigned type) returns a
sentinel error tuple (e.g. std::make_tuple(-1, 0ul, 0ul)), and then use
operator[] (or at() safely) for subsequent accesses; ensure callers can detect
the error via the negative int return and update any callers if needed.
Comment on lines
+170
to
+183
| if (m_min_eta.at(layer_index).at(eta_index) > eta_abs | ||
| || m_min_eta.at(layer_index).at(eta_index + 1) < eta_abs) | ||
| { | ||
| FCS_MSG_ERROR( | ||
| "Found eta outside of the specified eta range for the phi modulation"); | ||
| FCS_MSG_ERROR("Layer: " << layer_index); | ||
| FCS_MSG_ERROR("Eta: " << eta); | ||
| FCS_MSG_ERROR("Phi: " << phi); | ||
| FCS_MSG_ERROR("Eta min of bin: " | ||
| << m_min_eta.at(layer_index).at(eta_index) | ||
| << " Eta max of bin: " | ||
| << m_min_eta.at(layer_index).at(eta_index + 1)); | ||
| return std::make_tuple(2, 0, 0); // Error code 2: Eta outside of range | ||
| } |
There was a problem hiding this comment.
Dead / unreachable check after upper_bound.
By construction of std::upper_bound, once the eta_index >= size - 1 branch at L155 is passed:
m_min_eta[eta_index] <= eta_absis always true (so> eta_absat L170 is false), andm_min_eta[eta_index + 1] > eta_absis always true (so< eta_absat L171 is false).
This block therefore never triggers and can be removed, or the intent clarified (e.g., rejecting hits whose eta_abs falls into a gap between two configured ranges would need a stored "is-valid-range" flag, not these comparisons).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/Core/TFCSPhiModulationCorrection.cxx` around lines 170 - 183, The
eta-range check using m_min_eta.at(layer_index).at(eta_index) and
m_min_eta.at(layer_index).at(eta_index + 1) is unreachable after using
std::upper_bound to compute eta_index; remove this dead block from
TFCSPhiModulationCorrection.cxx (the error-logging + return tuple) or replace it
with an explicit validity check: add and consult a stored per-bin flag (e.g.,
m_eta_bin_valid[layer_index][eta_index]) to reject gaps if that was the intent.
Update references to eta_index and m_min_eta accordingly so the code either
drops the redundant bounds test or checks the new validity flag before returning
the error tuple.
Comment on lines
+41
to
+44
| TFCSVoxelHistoLateralCovarianceFluctuations:: | ||
| ~TFCSVoxelHistoLateralCovarianceFluctuations() | ||
| { | ||
| } |
There was a problem hiding this comment.
Release the owned transform functions.
initialize() allocates TFCS1DFunctionInt32Int32Histogram objects with new, but the destructor is empty. Long simulation jobs or repeated initialization will leak every per-voxel transform.
Minimal cleanup fix
TFCSVoxelHistoLateralCovarianceFluctuations::
~TFCSVoxelHistoLateralCovarianceFluctuations()
{
+ for (auto& transform_x : m_transform) {
+ for (auto& transform_y : transform_x) {
+ for (TFCS1DFunction* func : transform_y) {
+ delete func;
+ }
+ }
+ }
+ for (TH2* hist : m_voxel_template) {
+ delete hist;
+ }
}Also applies to: 129-135
Comment on lines
+78
to
+96
| TObject* obj = inputfile->Get(("parMeans" + label).c_str()); | ||
| if (!obj) { | ||
| FCS_MSG_ERROR("parMeans" + label + " not found"); | ||
| return false; | ||
| } | ||
| if (msgLvl(FCS_MSG::DEBUG)) { | ||
| FCS_MSG_DEBUG("parMeans"); | ||
| obj->Print(); | ||
| } | ||
| parMeans.ResizeTo(m_nDim_x * m_nDim_y); | ||
| parMeans = *dynamic_cast<TVectorT<double>*>(obj); | ||
| m_parMeans.push_back(parMeans); | ||
|
|
||
| obj = inputfile->Get(("covMatrix" + label).c_str()); | ||
| if (!obj) { | ||
| FCS_MSG_ERROR("covMatrix" + label + " not found"); | ||
| return false; | ||
| } | ||
| TMatrixTSym<double> covMatrix = *dynamic_cast<TMatrixTSym<double>*>(obj); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n source/Core/TFCSVoxelHistoLateralCovarianceFluctuations.cxx | sed -n '70,100p'Repository: fcs-proj/FastCaloSim
Length of output: 1384
🏁 Script executed:
# Check the context around this function to understand the full scope
wc -l source/Core/TFCSVoxelHistoLateralCovarianceFluctuations.cxxRepository: fcs-proj/FastCaloSim
Length of output: 127
🏁 Script executed:
# Let's see the function signature and understand what this is part of
sed -n '50,78p' source/Core/TFCSVoxelHistoLateralCovarianceFluctuations.cxx | cat -nRepository: fcs-proj/FastCaloSim
Length of output: 1214
🏁 Script executed:
# Check if there's any exception handling or other safeguards
sed -n '60,120p' source/Core/TFCSVoxelHistoLateralCovarianceFluctuations.cxx | cat -nRepository: fcs-proj/FastCaloSim
Length of output: 2549
🏁 Script executed:
# Check if similar patterns exist elsewhere in the file
rg "dynamic_cast.*\*\(obj\)" source/Core/TFCSVoxelHistoLateralCovarianceFluctuations.cxx -nRepository: fcs-proj/FastCaloSim
Length of output: 46
🏁 Script executed:
# Check the entire function to see if there's any broader context or pattern
sed -n '45,150p' source/Core/TFCSVoxelHistoLateralCovarianceFluctuations.cxx | cat -nRepository: fcs-proj/FastCaloSim
Length of output: 4279
Add null checks after dynamic_cast operations before dereferencing.
The code checks whether an object exists with inputfile->Get(), but this only validates the object's existence, not its type. At lines 44 and 52, dynamic_cast returns nullptr if the object has the wrong ROOT class, and dereferencing it causes undefined behavior. The safer pattern already used at line 16-21 (TH2* temp = dynamic_cast<...>(); if (!temp)) should be applied here.
Proposed fix
parMeans.ResizeTo(m_nDim_x * m_nDim_y);
- parMeans = *dynamic_cast<TVectorT<double>*>(obj);
+ auto* parMeansObj = dynamic_cast<TVectorT<double>*>(obj);
+ if (!parMeansObj) {
+ FCS_MSG_ERROR("parMeans" + label + " has unexpected type");
+ return false;
+ }
+ parMeans = *parMeansObj;
- TMatrixTSym<double> covMatrix = *dynamic_cast<TMatrixTSym<double>*>(obj);
+ auto* covMatrixObj = dynamic_cast<TMatrixTSym<double>*>(obj);
+ if (!covMatrixObj) {
+ FCS_MSG_ERROR("covMatrix" + label + " has unexpected type");
+ return false;
+ }
+ TMatrixTSym<double> covMatrix = *covMatrixObj;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/Core/TFCSVoxelHistoLateralCovarianceFluctuations.cxx` around lines 78
- 96, The dynamic_cast results for the objects retrieved from inputfile (the
TVectorT<double>* used to fill parMeans and the TMatrixTSym<double>* used to
initialize covMatrix) need null checks before dereferencing: after obtaining obj
for ("parMeans" + label) perform auto* vec =
dynamic_cast<TVectorT<double>*>(obj) and if (!vec) log an FCS_MSG_ERROR
including the label and return false, then assign parMeans = *vec and push to
m_parMeans; similarly after obtaining obj for ("covMatrix" + label) perform
auto* mat = dynamic_cast<TMatrixTSym<double>*>(obj) and if (!mat) log an
FCS_MSG_ERROR including the label and return false, then assign covMatrix =
*mat. Ensure the debug printing still uses obj->Print() only when the cast
succeeded (or use vec/mat for printing) and keep existing ResizeTo behavior for
parMeans.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (5)
source/Core/TFCSVoxelHistoLateralCovarianceFluctuations.cxx (5)
78-96:⚠️ Potential issue | 🟠 MajorCheck
dynamic_castresults before dereferencing.
Get()only proves the object exists, not that it has the expected ROOT type. Both casts can returnnullptr, and the immediate dereference would crash on malformed or mismatched input files.Proposed fix
- parMeans.ResizeTo(m_nDim_x * m_nDim_y); - parMeans = *dynamic_cast<TVectorT<double>*>(obj); + auto* parMeansObj = dynamic_cast<TVectorT<double>*>(obj); + if (!parMeansObj) { + FCS_MSG_ERROR("parMeans" << label << " has unexpected type"); + return false; + } + parMeans.ResizeTo(m_nDim_x * m_nDim_y); + parMeans = *parMeansObj; m_parMeans.push_back(parMeans); obj = inputfile->Get(("covMatrix" + label).c_str()); @@ - TMatrixTSym<double> covMatrix = *dynamic_cast<TMatrixTSym<double>*>(obj); + auto* covMatrixObj = dynamic_cast<TMatrixTSym<double>*>(obj); + if (!covMatrixObj) { + FCS_MSG_ERROR("covMatrix" << label << " has unexpected type"); + return false; + } + TMatrixTSym<double> covMatrix = *covMatrixObj;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/Core/TFCSVoxelHistoLateralCovarianceFluctuations.cxx` around lines 78 - 96, The code dereferences the results of dynamic_cast without checking for null, which can crash if the ROOT object isn't the expected type; update the block that reads ("parMeans"+label) and ("covMatrix"+label) to perform a safe dynamic_cast to TVectorT<double>* and TMatrixTSym<double>* respectively, check each cast result for nullptr, log an FCS_MSG_ERROR including the label when the cast fails, return false on failure, and only call parMeans.ResizeTo, assign parMeans, push_back to m_parMeans, or construct covMatrix after the corresponding cast has succeeded.
41-44:⚠️ Potential issue | 🟠 MajorRelease the owned transform functions.
initialize()allocates oneTFCS1DFunctionInt32Int32Histogramper voxel/bin, but the destructor never frees them. Repeated initialization or long jobs will leak these objects.Minimal cleanup
TFCSVoxelHistoLateralCovarianceFluctuations:: ~TFCSVoxelHistoLateralCovarianceFluctuations() { + for (auto& transform_x : m_transform) { + for (auto& transform_y : transform_x) { + for (TFCS1DFunction* func : transform_y) { + delete func; + } + } + } + for (TH2* hist : m_voxel_template) { + delete hist; + } }Also applies to: 129-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/Core/TFCSVoxelHistoLateralCovarianceFluctuations.cxx` around lines 41 - 44, The destructor ~TFCSVoxelHistoLateralCovarianceFluctuations currently doesn't delete the TFCS1DFunctionInt32Int32Histogram objects allocated in initialize(); update the destructor (and the duplicate destructor area around the 129-135 region) to iterate the container that holds those pointers (e.g. the member storing per-voxel/bin functions such as m_transforms or m_functionPointers), delete each TFCS1DFunctionInt32Int32Histogram*, set pointers to nullptr (or clear the container), and ensure the container is cleared to avoid leaks on repeated initialize() calls.
60-67:⚠️ Potential issue | 🟠 MajorDo not keep a
TFile-owned template histogram.The histogram returned by
Get()is owned by the input file.m_voxel_template[0]is later cloned insimulate(), so closing the file first can leave a dangling pointer here. Clone and detach the template during initialization.Proposed fix
- m_voxel_template.push_back(temp); + TH2* template_clone = dynamic_cast<TH2*>(temp->Clone()); + if (!template_clone) { + FCS_MSG_ERROR("Failed to clone template hist for cs " + std::to_string(cs)); + return false; + } + template_clone->SetDirectory(nullptr); + m_voxel_template.push_back(template_clone);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/Core/TFCSVoxelHistoLateralCovarianceFluctuations.cxx` around lines 60 - 67, The code currently stores the TFile-owned histogram pointer returned by inputfile->Get into m_voxel_template (temp), which will dangle when the file is closed; instead, clone and detach the histogram during initialization: after obtaining temp from inputfile->Get(Form(...)) create a detached clone (use temp->Clone(...) and then call SetDirectory(nullptr) on the clone or otherwise detach it from the file), push the clone into m_voxel_template (not the original temp), and use that clone to set m_nDim_x; keep the original temp ownership with the TFile untouched so closing the file won’t leave dangling pointers and simulate() can safely clone the detached template.
242-243:⚠️ Potential issue | 🟠 MajorUse the standard normal CDF mapping.
Erf(x * 2 / pi)is not the Gaussian CDF transform. This biases every voxel weight sampled fromgenPars.Proposed fix
- double cdf_val = - (TMath::Erf(genPars[count] * 2.0 / TMath::Pi()) + 1) / 2.0; + double cdf_val = + 0.5 * (1.0 + TMath::Erf(genPars[count] / TMath::Sqrt(2.0)));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/Core/TFCSVoxelHistoLateralCovarianceFluctuations.cxx` around lines 242 - 243, The code computes cdf_val using an incorrect transform (Erf(genPars[count] * 2.0 / TMath::Pi())), which is not the standard normal CDF and biases sampled voxel weights; replace that expression with the standard normal CDF mapping, e.g. cdf_val = 0.5 * (1.0 + TMath::Erf(genPars[count] / TMath::Sqrt(2.0))) so that cdf_val correctly represents Phi(genPars[count]) for the TFCSVoxelHistoLateralCovarianceFluctuations logic (refer to the cdf_val variable and genPars[count] in the file).
219-223:⚠️ Potential issue | 🔴 CriticalValidate
Ebinagainst every PCA table before indexing.The current check only covers
m_parMeans, but the sameEbin - 1index is used form_transform,m_EigenValues, andm_EigenVectors. A partially loaded input file can still go out of bounds.Proposed fix
- if (static_cast<size_t>(Ebin) > m_parMeans.size()) { + if (static_cast<size_t>(Ebin) > m_parMeans.size() + || static_cast<size_t>(Ebin) > m_transform.size() + || static_cast<size_t>(Ebin) > m_EigenValues.size() + || static_cast<size_t>(Ebin) > m_EigenVectors.size()) { FCS_MSG_ERROR("Ebin " << Ebin << " exceeds loaded PCA tables (size " << m_parMeans.size() << ")"); return FCSFatal; }Also applies to: 246-246
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/Core/TFCSVoxelHistoLateralCovarianceFluctuations.cxx` around lines 219 - 223, The bounds check only verifies m_parMeans but later indexes Ebin-1 into m_transform, m_EigenValues and m_EigenVectors, so add a single authoritative validation that Ebin is within range for all PCA tables before any indexing: ensure Ebin is >= 1 and that static_cast<size_t>(Ebin-1) < m_parMeans.size(), m_transform.size(), m_EigenValues.size() and m_EigenVectors.size(); if any fail, log an error referencing Ebin and the offending container(s) (use FCS_MSG_ERROR) and return FCSFatal. This replaces the existing m_parMeans-only check around Ebin and should also be applied at the other spot noted (around the code at line ~246).
🧹 Nitpick comments (3)
source/Core/TFCSBinnedShowerONNX.cxx (2)
534-605: Samestd::cerr/std::coutand RAII concerns asTFCSBinnedShower::load_sub_bin_distribution.This is a near-verbatim copy of the non-ONNX loader and shares its logging/ownership issues. Preferably deduplicate (see the file-level refactor suggestion) and, in the meantime, route messages through
FCS_MSG_*and usestd::unique_ptr<TFile>for the file lifetime.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/Core/TFCSBinnedShowerONNX.cxx` around lines 534 - 605, TFCSBinnedShowerONNX::load_sub_bin_distribution duplicates the non-ONNX loader by using std::cerr/std::cout for diagnostics and manual TFile deletion; replace those with the project logging macros (use FCS_MSG_ERROR/FCS_MSG_INFO or similar instead of std::cerr/std::cout) and manage the TFile with RAII (std::unique_ptr<TFile> with a custom deleter or std::unique_ptr<TFile, decltype(&::Close)>) so the file is closed and deleted automatically; keep the rest of the parsing logic intact and ensure m_upscaling_energies and m_sub_bin_distribution are assigned after successful load.
1-605: Large-scale duplication withTFCSBinnedShower.cxx.
compute_n_hits_and_elayer,get_n_hits,get_layer_energy,get_energy_index,get_coordinates,upscale,delete_event,load_hdf5_dataset,load_bin_boundaries, andload_sub_bin_distributionare essentially copy-pasted fromTFCSBinnedShower.cxxwith only the event/layer type substituted (plus a one-liner layer-1 vs layer-0 comment difference inupscale). This makes future fixes (like the duplicate-tagged issues on empty-vectorback(),bin_indexbounds, and null-pointer guards) need to be applied twice and silently drift.Consider hoisting the shared logic into
TFCSBinnedShowerBase(the base already declares the virtual hooks) — e.g. makeevent_t/layer_ta common POD type or a templated member and implement the coordinate/hit-count bookkeeping once. This would keepTFCSBinnedShowerandTFCSBinnedShowerONNXas thin "how do I get the next event" strategies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/Core/TFCSBinnedShowerONNX.cxx` around lines 1 - 605, The file duplicates large portions of TFCSBinnedShower.cxx (compute_n_hits_and_elayer, get_n_hits, get_layer_energy, get_energy_index, get_coordinates, upscale, delete_event, load_hdf5_dataset, load_bin_boundaries, load_sub_bin_distribution) leading to maintenance drift; refactor by moving the shared logic into TFCSBinnedShowerBase (or a new common helper) and have TFCSBinnedShowerONNX call the common implementations: extract the shared routines into non-virtual protected methods in TFCSBinnedShowerBase (or templated helpers) that operate on a common event_t/layer_t POD or adapter interface, replace the copy-pasted bodies in TFCSBinnedShowerONNX with calls to those base helpers (preserve TFCSBinnedShowerONNX::get_event-specific code and only adapt accessors to use the base methods), and update get_event/delete_event to set/get the same aux keys so the common routines (e.g. compute_n_hits_and_elayer, get_energy_index, get_coordinates, upscale, load_hdf5_dataset, load_bin_boundaries, load_sub_bin_distribution) operate on both simulators without duplication.source/Core/TFCSBinnedShower.cxx (1)
807-876: UseFCS_MSG_*logging and avoid straystd::cerr/std::coutin a library loader.
load_sub_bin_distributionwrites tostd::cerron failure (line 812) and unconditionally dumps a per-layer shape summary tostd::cout(lines 865–873). Neither is controlled by theISF_FCS::MLoggingverbosity used everywhere else in the class, so this is noisy at runtime and inconsistent with the surrounding code style. Also consider usingstd::unique_ptr<TFile>/TFile::Closein an RAII wrapper rather than thenew/deletepair with manualClose, which leaks if any of the intermediate calls throws.♻️ Proposed adjustment
- TFile* file = TFile::Open(filename.c_str(), "READ"); - if (!file || file->IsZombie()) { - std::cerr << "Failed to open file: " << filename << std::endl; - return; - } + std::unique_ptr<TFile> file(TFile::Open(filename.c_str(), "READ")); + if (!file || file->IsZombie()) { + FCS_MSG_ERROR("Failed to open sub-bin distribution file: " << filename); + return; + } ... - for (size_t i = 0; i < energies.size(); ++i) { - std::cout << "Energy index " << i << ": " << energies[i] << " GeV\n"; - for (size_t j = 0; j < data[i].size(); ++j) { - if (!data[i][j].empty()) { - std::cout << " Layer " << j << " Shape: (" << data[i][j].size() << ", " - << data[i][j][0].size() << ")\n"; - } - } - } + for (size_t i = 0; i < energies.size(); ++i) { + FCS_MSG_DEBUG("Energy index " << i << ": " << energies[i] << " GeV"); + ... + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/Core/TFCSBinnedShower.cxx` around lines 807 - 876, The loader TFCSBinnedShower::load_sub_bin_distribution currently prints errors with std::cerr and dumps shapes with std::cout; replace those with the library logging macros (e.g. FCS_MSG_ERROR/FCS_MSG_WARNING/FCS_MSG_INFO or FCS_MSG_DEBUG as appropriate via ISF_FCS::MLogging) and gate the per-energy/per-layer shape summary behind a debug/info log condition rather than unconditional cout; also replace the manual TFile* new/Close/delete with an RAII wrapper (std::unique_ptr<TFile> with a custom deleter that calls ->Close() and deletes) so the file is always closed on exceptions. Ensure you update the uses around load_sub_bin_distribution, keep setting m_upscaling_energies and m_sub_bin_distribution unchanged, and reference the function name load_sub_bin_distribution and members m_upscaling_energies/m_sub_bin_distribution when making the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/FastCaloSim/Core/TFCSVoxelHistoLateralCovarianceFluctuations.h`:
- Line 24: The override of set_geometry in
TFCSVoxelHistoLateralCovarianceFluctuations currently only assigns m_geo and
skips the base propagation; change the method to keep m_geo = geo but also call
TFCSParametrizationBase::set_geometry(geo) so daughter parametrizations receive
the geometry pointer (i.e., inside
TFCSVoxelHistoLateralCovarianceFluctuations::set_geometry call the base-class
TFCSParametrizationBase::set_geometry(geo) after or before assigning m_geo).
In `@source/Core/TFCSBinnedShower.cxx`:
- Around line 779-805: TFCSBinnedShower::Streamer currently dereferences m_geo
(calling m_geo->n_layers()) while m_geo is transient and can be nullptr after
ReadClassBuffer; add a null-check before using m_geo so you only build the
layers vector and call load_event_library(m_hdf5_file, layers) when m_geo !=
nullptr (and m_hdf5_file is non-empty); if m_geo is nullptr but m_hdf5_file is
non-empty, skip the reload (and optionally log a warning) so you avoid accessing
m_geo and prevent a segfault, leaving m_eventlibrary untouched until geometry is
injected.
In `@source/Core/TFCSMLCalorimeterSimulator.cxx`:
- Around line 104-129: The retry loop currently breaks when the retry budget is
exhausted but may still have NaNs in `outputs`, which then fall through and
produce a biased event; change the logic in the `while (has_nan(outputs))` /
`retry` block (the `has_nan` lambda and `predictVoxels` call) so that if retries
are exhausted and `has_nan(outputs)` is still true you return `event_t{}` (same
as the empty-output branch) instead of breaking and continuing; alternatively
perform a post-loop check of `has_nan(outputs)` and return `event_t{}` when true
to ensure no NaN-containing outputs are used to build an event.
In `@source/Core/TFCSVoxelHistoLateralCovarianceFluctuations.cxx`:
- Around line 187-189: m_geo is dereferenced before the method's
early-exit/no-op handling for uninitialized PCA runs, causing a crash; add a
null-check for m_geo before calling m_geo->n_layers() (and before any later uses
around the block where Ebin and the for-loop over ilayer are declared) and
perform the same early-return/fatal-handling used for the uninitialized PCA path
if m_geo is null so that the loop using m_geo->n_layers() only executes when
m_geo is valid (also apply the same guard to the subsequent occurrences around
the later block referenced in the comment).
- Around line 123-131: Replace the unsafe C-style cast when fetching the
histogram with a runtime-checked dynamic_cast: after building histname and
calling inputfile->Get(histname.c_str()), use dynamic_cast<TH1*> on the returned
TObject* and check for nullptr; if the dynamic_cast returns nullptr, log the
same FCS_MSG_ERROR("Histogram " << histname << " not found") and return false,
otherwise pass the safely cast TH1* to TFCS1DFunctionInt32Int32Histogram via
func->Initialize(hist, false).
- Line 54: After calling inputfile->cd(folder.c_str()) in
TFCSVoxelHistoLateralCovarianceFluctuations.cxx, replace subsequent uses of
inputfile->Get(...) (the object retrievals performed after the cd) with
gDirectory->Get(...) so the lookup uses the current ROOT directory;
alternatively obtain a TDirectory* with inputfile->GetDirectory(folder.c_str())
and call dir->Get(...) to avoid relying on global state—update the retrievals
that reference inputfile->Get(...) (following the inputfile->cd(folder.c_str())
call) to one of these patterns.
---
Duplicate comments:
In `@source/Core/TFCSVoxelHistoLateralCovarianceFluctuations.cxx`:
- Around line 78-96: The code dereferences the results of dynamic_cast without
checking for null, which can crash if the ROOT object isn't the expected type;
update the block that reads ("parMeans"+label) and ("covMatrix"+label) to
perform a safe dynamic_cast to TVectorT<double>* and TMatrixTSym<double>*
respectively, check each cast result for nullptr, log an FCS_MSG_ERROR including
the label when the cast fails, return false on failure, and only call
parMeans.ResizeTo, assign parMeans, push_back to m_parMeans, or construct
covMatrix after the corresponding cast has succeeded.
- Around line 41-44: The destructor ~TFCSVoxelHistoLateralCovarianceFluctuations
currently doesn't delete the TFCS1DFunctionInt32Int32Histogram objects allocated
in initialize(); update the destructor (and the duplicate destructor area around
the 129-135 region) to iterate the container that holds those pointers (e.g. the
member storing per-voxel/bin functions such as m_transforms or
m_functionPointers), delete each TFCS1DFunctionInt32Int32Histogram*, set
pointers to nullptr (or clear the container), and ensure the container is
cleared to avoid leaks on repeated initialize() calls.
- Around line 60-67: The code currently stores the TFile-owned histogram pointer
returned by inputfile->Get into m_voxel_template (temp), which will dangle when
the file is closed; instead, clone and detach the histogram during
initialization: after obtaining temp from inputfile->Get(Form(...)) create a
detached clone (use temp->Clone(...) and then call SetDirectory(nullptr) on the
clone or otherwise detach it from the file), push the clone into
m_voxel_template (not the original temp), and use that clone to set m_nDim_x;
keep the original temp ownership with the TFile untouched so closing the file
won’t leave dangling pointers and simulate() can safely clone the detached
template.
- Around line 242-243: The code computes cdf_val using an incorrect transform
(Erf(genPars[count] * 2.0 / TMath::Pi())), which is not the standard normal CDF
and biases sampled voxel weights; replace that expression with the standard
normal CDF mapping, e.g. cdf_val = 0.5 * (1.0 + TMath::Erf(genPars[count] /
TMath::Sqrt(2.0))) so that cdf_val correctly represents Phi(genPars[count]) for
the TFCSVoxelHistoLateralCovarianceFluctuations logic (refer to the cdf_val
variable and genPars[count] in the file).
- Around line 219-223: The bounds check only verifies m_parMeans but later
indexes Ebin-1 into m_transform, m_EigenValues and m_EigenVectors, so add a
single authoritative validation that Ebin is within range for all PCA tables
before any indexing: ensure Ebin is >= 1 and that static_cast<size_t>(Ebin-1) <
m_parMeans.size(), m_transform.size(), m_EigenValues.size() and
m_EigenVectors.size(); if any fail, log an error referencing Ebin and the
offending container(s) (use FCS_MSG_ERROR) and return FCSFatal. This replaces
the existing m_parMeans-only check around Ebin and should also be applied at the
other spot noted (around the code at line ~246).
---
Nitpick comments:
In `@source/Core/TFCSBinnedShower.cxx`:
- Around line 807-876: The loader TFCSBinnedShower::load_sub_bin_distribution
currently prints errors with std::cerr and dumps shapes with std::cout; replace
those with the library logging macros (e.g.
FCS_MSG_ERROR/FCS_MSG_WARNING/FCS_MSG_INFO or FCS_MSG_DEBUG as appropriate via
ISF_FCS::MLogging) and gate the per-energy/per-layer shape summary behind a
debug/info log condition rather than unconditional cout; also replace the manual
TFile* new/Close/delete with an RAII wrapper (std::unique_ptr<TFile> with a
custom deleter that calls ->Close() and deletes) so the file is always closed on
exceptions. Ensure you update the uses around load_sub_bin_distribution, keep
setting m_upscaling_energies and m_sub_bin_distribution unchanged, and reference
the function name load_sub_bin_distribution and members
m_upscaling_energies/m_sub_bin_distribution when making the changes.
In `@source/Core/TFCSBinnedShowerONNX.cxx`:
- Around line 534-605: TFCSBinnedShowerONNX::load_sub_bin_distribution
duplicates the non-ONNX loader by using std::cerr/std::cout for diagnostics and
manual TFile deletion; replace those with the project logging macros (use
FCS_MSG_ERROR/FCS_MSG_INFO or similar instead of std::cerr/std::cout) and manage
the TFile with RAII (std::unique_ptr<TFile> with a custom deleter or
std::unique_ptr<TFile, decltype(&::Close)>) so the file is closed and deleted
automatically; keep the rest of the parsing logic intact and ensure
m_upscaling_energies and m_sub_bin_distribution are assigned after successful
load.
- Around line 1-605: The file duplicates large portions of TFCSBinnedShower.cxx
(compute_n_hits_and_elayer, get_n_hits, get_layer_energy, get_energy_index,
get_coordinates, upscale, delete_event, load_hdf5_dataset, load_bin_boundaries,
load_sub_bin_distribution) leading to maintenance drift; refactor by moving the
shared logic into TFCSBinnedShowerBase (or a new common helper) and have
TFCSBinnedShowerONNX call the common implementations: extract the shared
routines into non-virtual protected methods in TFCSBinnedShowerBase (or
templated helpers) that operate on a common event_t/layer_t POD or adapter
interface, replace the copy-pasted bodies in TFCSBinnedShowerONNX with calls to
those base helpers (preserve TFCSBinnedShowerONNX::get_event-specific code and
only adapt accessors to use the base methods), and update get_event/delete_event
to set/get the same aux keys so the common routines (e.g.
compute_n_hits_and_elayer, get_energy_index, get_coordinates, upscale,
load_hdf5_dataset, load_bin_boundaries, load_sub_bin_distribution) operate on
both simulators without duplication.
🪄 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: 495d097e-da70-40c9-9dc0-3ce697c57845
📒 Files selected for processing (22)
CMakeLists.txtcmake/docs-ci.cmakecmake/lint.cmakecmake/spell.cmakeinclude/FastCaloSim/Core/TFCSBinnedShower.hinclude/FastCaloSim/Core/TFCSBinnedShowerBase.hinclude/FastCaloSim/Core/TFCSBinnedShowerONNX.hinclude/FastCaloSim/Core/TFCSMLCalorimeterSimulator.hinclude/FastCaloSim/Core/TFCSPhiModulationCorrection.hinclude/FastCaloSim/Core/TFCSVoxelHistoLateralCovarianceFluctuations.hparam/CMakeLists.txtsource/Core/TFCSBinnedShower.cxxsource/Core/TFCSBinnedShowerBase.cxxsource/Core/TFCSBinnedShowerONNX.cxxsource/Core/TFCSLateralShapeParametrizationFixedHitChain.cxxsource/Core/TFCSMLCalorimeterSimulator.cxxsource/Core/TFCSPhiModulationCorrection.cxxsource/Core/TFCSVoxelHistoLateralCovarianceFluctuations.cxxtest/CMakeLists.txttest/TestPlugins/ATLAS/ATLASFCalGeoPlugin/CMakeLists.txttest/TestPlugins/ATLAS/ATLASMagneticFieldMapPlugin/CMakeLists.txttest/g4app/CMakeLists.txt
✅ Files skipped from review due to trivial changes (8)
- cmake/docs-ci.cmake
- test/TestPlugins/ATLAS/ATLASMagneticFieldMapPlugin/CMakeLists.txt
- cmake/lint.cmake
- test/TestPlugins/ATLAS/ATLASFCalGeoPlugin/CMakeLists.txt
- test/g4app/CMakeLists.txt
- test/CMakeLists.txt
- cmake/spell.cmake
- param/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (6)
- CMakeLists.txt
- include/FastCaloSim/Core/TFCSBinnedShowerBase.h
- include/FastCaloSim/Core/TFCSBinnedShowerONNX.h
- include/FastCaloSim/Core/TFCSPhiModulationCorrection.h
- include/FastCaloSim/Core/TFCSBinnedShower.h
- source/Core/TFCSPhiModulationCorrection.cxx
| const char* title = nullptr); | ||
| virtual ~TFCSVoxelHistoLateralCovarianceFluctuations(); | ||
|
|
||
| virtual void set_geometry(CaloGeo* geo) override { m_geo = geo; }; |
There was a problem hiding this comment.
Propagate geometry to daughter parametrizations.
This override bypasses TFCSParametrizationBase::set_geometry, whose implementation propagates the geometry pointer to daughter objects. Keep the local assignment, but call the base implementation as well.
Proposed fix
- virtual void set_geometry(CaloGeo* geo) override { m_geo = geo; };
+ virtual void set_geometry(CaloGeo* geo) override
+ {
+ m_geo = geo;
+ TFCSLateralShapeParametrizationHitBase::set_geometry(geo);
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| virtual void set_geometry(CaloGeo* geo) override { m_geo = geo; }; | |
| virtual void set_geometry(CaloGeo* geo) override | |
| { | |
| m_geo = geo; | |
| TFCSLateralShapeParametrizationHitBase::set_geometry(geo); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/FastCaloSim/Core/TFCSVoxelHistoLateralCovarianceFluctuations.h` at
line 24, The override of set_geometry in
TFCSVoxelHistoLateralCovarianceFluctuations currently only assigns m_geo and
skips the base propagation; change the method to keep m_geo = geo but also call
TFCSParametrizationBase::set_geometry(geo) so daughter parametrizations receive
the geometry pointer (i.e., inside
TFCSVoxelHistoLateralCovarianceFluctuations::set_geometry call the base-class
TFCSParametrizationBase::set_geometry(geo) after or before assigning m_geo).
Comment on lines
+779
to
+805
| if (R__b.IsReading()) { | ||
| R__b.ReadClassBuffer(TFCSBinnedShower::Class(), this); | ||
|
|
||
| // Load the event library from file | ||
| if (!m_hdf5_file.empty()) { | ||
| std::vector<long unsigned int> layers; | ||
| for (long unsigned int i = 0; i < m_geo->n_layers(); ++i) { | ||
| layers.push_back(i); | ||
| } | ||
| FCS_MSG_INFO("Loading event library from " << m_hdf5_file); | ||
| ((TFCSBinnedShower*)this)->load_event_library(m_hdf5_file, layers); | ||
| FCS_MSG_INFO("Size after loading" << m_eventlibrary.size()); | ||
| } else { | ||
| FCS_MSG_INFO( | ||
| "Using existing event library of size: " << m_eventlibrary.size()); | ||
| } | ||
|
|
||
| } else { | ||
| // Remove the event library again, if it was loaded from file | ||
| if (!m_hdf5_file.empty()) { | ||
| FCS_MSG_DEBUG("Clear event library before saving"); | ||
| m_eventlibrary.resize(0); | ||
| } | ||
|
|
||
| R__b.WriteClassBuffer(TFCSBinnedShower::Class(), this); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify m_geo is a non-persistified pointer and confirm no code sets it before Streamer reads
rg -nP --type=cpp --type=hpp -C2 '\bm_geo\b'
rg -nP -C2 'CaloGeo\s*\*\s*m_geo'Repository: fcs-proj/FastCaloSim
Length of output: 4177
🏁 Script executed:
# Locate and examine the TFCSBinnedShower.cxx file
find . -name "TFCSBinnedShower.cxx" -o -name "TFCSBinnedShower.h" | head -20Repository: fcs-proj/FastCaloSim
Length of output: 144
🏁 Script executed:
# Get the size and read lines around 779-805 in TFCSBinnedShower.cxx
wc -l source/Core/TFCSBinnedShower.cxx
sed -n '770,815p' source/Core/TFCSBinnedShower.cxxRepository: fcs-proj/FastCaloSim
Length of output: 1403
🏁 Script executed:
# Check the TFCSBinnedShower class header to see m_geo and m_hdf5_file declarations
grep -n "m_geo\|m_hdf5_file" include/FastCaloSim/Core/TFCSBinnedShower.h -B2 -A1Repository: fcs-proj/FastCaloSim
Length of output: 519
🏁 Script executed:
# Search for how m_geo is initialized/set in TFCSBinnedShower
rg -n "m_geo\s*=" source/Core/TFCSBinnedShower.cxx | head -20Repository: fcs-proj/FastCaloSim
Length of output: 46
🏁 Script executed:
# Check TFCSBinnedShower class definition and inheritance
grep -n "class TFCSBinnedShower" include/FastCaloSim/Core/TFCSBinnedShower.h -A5Repository: fcs-proj/FastCaloSim
Length of output: 213
🏁 Script executed:
# Look for setters or methods that set m_geo
grep -n "set_geo\|m_geo\s*=" include/FastCaloSim/Core/TFCSBinnedShower.hRepository: fcs-proj/FastCaloSim
Length of output: 46
🏁 Script executed:
# Search for SetGeo or similar in the entire codebase
rg -n "SetGeo|set_geo" --type=cpp --type=h | grep -i "m_geo"Repository: fcs-proj/FastCaloSim
Length of output: 1151
🏁 Script executed:
# Look at TFCSBinnedShowerBase to understand how m_geo is used and set
grep -n "m_geo\|SetGeo\|set_geo" include/FastCaloSim/Core/TFCSBinnedShowerBase.hRepository: fcs-proj/FastCaloSim
Length of output: 290
🏁 Script executed:
# Check how TFCSBinnedShower objects are typically created/deserialized
grep -n "ReadClassBuffer\|Streamer" include/FastCaloSim/Core/TFCSBinnedShower.h -B2 -A2Repository: fcs-proj/FastCaloSim
Length of output: 46
🏁 Script executed:
# Look for any ROOT TClass setup or Streamer declarations
grep -n "ClassDef\|Streamer" include/FastCaloSim/Core/TFCSBinnedShower.hRepository: fcs-proj/FastCaloSim
Length of output: 128
🏁 Script executed:
# Check if there are other Streamers in the codebase that have similar patterns/guards
rg -n "Streamer.*TBuffer" include/FastCaloSim/Core/ -A15 | grep -E "m_geo|nullptr|guard|if.*m_" | head -30Repository: fcs-proj/FastCaloSim
Length of output: 225
🏁 Script executed:
# Final check: verify the exact line numbers match and get context around line 785
sed -n '780,790p' source/Core/TFCSBinnedShower.cxxRepository: fcs-proj/FastCaloSim
Length of output: 550
Add null check guard before dereferencing m_geo in Streamer.
m_geo is transient (marked //! do not persistify in TFCSBinnedShowerBase), so ReadClassBuffer does not restore it. Meanwhile, m_hdf5_file is persisted and will be restored if the file contained it. When m_hdf5_file is non-empty but m_geo is still nullptr (typical when loading a fresh parametrization before geometry injection), line 785 calls m_geo->n_layers() and segfaults.
Guard the reload with a null check on m_geo:
Proposed guard
// Load the event library from file
if (!m_hdf5_file.empty()) {
+ if (!m_geo) {
+ FCS_MSG_ERROR(
+ "Cannot reload event library from "
+ << m_hdf5_file
+ << " during Streamer: geometry (m_geo) not yet set. "
+ << "Defer load_event_library() until after geometry is attached.");
+ return;
+ }
std::vector<long unsigned int> layers;
for (long unsigned int i = 0; i < m_geo->n_layers(); ++i) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/Core/TFCSBinnedShower.cxx` around lines 779 - 805,
TFCSBinnedShower::Streamer currently dereferences m_geo (calling
m_geo->n_layers()) while m_geo is transient and can be nullptr after
ReadClassBuffer; add a null-check before using m_geo so you only build the
layers vector and call load_event_library(m_hdf5_file, layers) when m_geo !=
nullptr (and m_hdf5_file is non-empty); if m_geo is nullptr but m_hdf5_file is
non-empty, skip the reload (and optionally log a warning) so you avoid accessing
m_geo and prevent a segfault, leaving m_eventlibrary untouched until geometry is
injected.
Comment on lines
+104
to
+129
| // Check if any output contains NaN; retry up to 5 times if so | ||
| auto has_nan = [](const VNetworkBase::NetworkOutputs& out) | ||
| { | ||
| for (const auto& kv : out) { | ||
| if (std::isnan(kv.second)) | ||
| return true; | ||
| } | ||
| return false; | ||
| }; | ||
|
|
||
| int retry = 0; | ||
| while (has_nan(outputs)) { | ||
| if (retry >= 5) { | ||
| FCS_MSG_WARNING("Network output still contains NaN after " | ||
| << retry << " retries. Giving up."); | ||
| break; | ||
| } | ||
| FCS_MSG_WARNING("Network output contains NaN. Retrying (attempt " | ||
| << retry + 1 << ")."); | ||
| outputs = predictVoxels(simulstate, eta, energy); | ||
| if (outputs.empty()) { | ||
| FCS_MSG_ERROR("predictVoxels returned empty outputs on retry; giving up"); | ||
| return event_t {}; | ||
| } | ||
| retry++; | ||
| } |
There was a problem hiding this comment.
Post-retry path silently accepts output with NaNs.
When the retry budget is exhausted and has_nan(outputs) is still true, the while loop breaks and the code falls through to build an event from the partially-NaN output (line 158 voxel_energy > 0 skips NaNs but cannot distinguish "no energy" from "failed inference"). Returning an empty event_t{} after the retry budget, mirroring the empty-output branch, would avoid producing a biased event from corrupted inference data.
🛡️ Suggested change
while (has_nan(outputs)) {
if (retry >= 5) {
- FCS_MSG_WARNING("Network output still contains NaN after "
- << retry << " retries. Giving up.");
- break;
+ FCS_MSG_ERROR("Network output still contains NaN after "
+ << retry << " retries. Returning empty event.");
+ return event_t {};
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/Core/TFCSMLCalorimeterSimulator.cxx` around lines 104 - 129, The retry
loop currently breaks when the retry budget is exhausted but may still have NaNs
in `outputs`, which then fall through and produce a biased event; change the
logic in the `while (has_nan(outputs))` / `retry` block (the `has_nan` lambda
and `predictVoxels` call) so that if retries are exhausted and
`has_nan(outputs)` is still true you return `event_t{}` (same as the
empty-output branch) instead of breaking and continuing; alternatively perform a
post-loop check of `has_nan(outputs)` and return `event_t{}` when true to ensure
no NaN-containing outputs are used to build an event.
| // m_transform[i][j]=new TFCS1DFunctionInt16Int16Histogram(hist) | ||
| // where hist is the 1D histogram from which a CDF was calculated | ||
|
|
||
| inputfile->cd(folder.c_str()); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
ROOT TFile::cd subdirectory TFile::Get current directory behavior member Get versus gDirectory Get
💡 Result:
In ROOT, TFile inherits from TDirectoryFile, which inherits from TDirectory. The key behaviors for handling subdirectories and current directory are: TFile::cd("subdir") or gDirectory->cd("subdir"): Changes the current directory (gDirectory) to the specified subdirectory. This loads the subdirectory into memory if needed and sets gDirectory to point to it. Path syntax supports absolute (file.root:/dir1/dir2) and relative (../dir) paths. Returns kTRUE on success. After cd("subdir"), gDirectory points to that subdirectory, and TDirectory::pwd or gDirectory->pwd shows the current path. TDirectory::Get("subdir") or TFile::Get("subdir"): Intended for retrieving objects (TObject-derived), not directories. Older documentation warned against using Get for TDirectory namecycles as it could lead to issues like segmentation violations when accessing newly created subdirectories without proper loading. Modern docs (e.g., v6+) show Get works for subdirectories (returns TObject*), and source code recursively calls GetDirectory for paths starting with '/', but direct use on subdirectory names may return the TKey or cause inconsistencies. Use TDirectory::GetDirectory("subdir") instead for directories, which explicitly finds and loads the subdirectory without changing gDirectory. gDirectory: Global (thread-local) pointer to the current directory. Changed by cd, opening files (sets to the file), or direct assignment. Many operations (e.g., Write, new histograms) use gDirectory by default. Best practices: - To work in a subdirectory without changing global state: Use TDirectory::TContext ctx(parent, subdir); - To get subdirectory pointer: parent->GetDirectory("subdir"); - Avoid Get for directories; prefer GetDirectory. Examples: TFile *f = TFile::Open("file.root"); f->cd("subdir"); // gDirectory now "file.root:/subdir" TH1 h = (TH1)gDirectory->Get("hist"); // Gets from subdir TDirectory *sub = f->GetDirectory("subdir"); // Pointer without cd
Citations:
- 1: https://root.cern.ch/doc/master/classTDirectory.html
- 2: https://root.cern/doc/v628/classTDirectory.html
- 3: https://root.cern.ch/doc/v628/classTDirectoryFile.html
- 4: https://root.cern/manual/io/
- 5: https://root.cern/doc/v638/classTDirectory.html
- 6: https://root.cern/root/html51904/TFile.html
- 7: https://root.cern/root/html524/TDirectory.html
- 8: https://root.cern/root/html602/TDirectory.html
- 9: https://root.cern/doc/v624/TDirectoryFile_8cxx_source.html
- 10: https://root.cern.ch/root/roottalk/roottalk98/1941.html
- 11: https://root.cern.ch/doc/v628/classTDirectory.html
- 12: https://root.cern.ch/root/roottalk/roottalk03/2050.html
🏁 Script executed:
# First, let's find the file and examine the context around line 54
find . -name "TFCSVoxelHistoLateralCovarianceFluctuations.cxx" -type fRepository: fcs-proj/FastCaloSim
Length of output: 125
🏁 Script executed:
# Read the file to see context around line 54
cat -n ./source/Core/TFCSVoxelHistoLateralCovarianceFluctuations.cxx | head -100 | tail -60Repository: fcs-proj/FastCaloSim
Length of output: 2489
🏁 Script executed:
# Continue reading to see the rest of the method
cat -n ./source/Core/TFCSVoxelHistoLateralCovarianceFluctuations.cxx | sed -n '100,180p'Repository: fcs-proj/FastCaloSim
Length of output: 3393
🏁 Script executed:
# Check for ROOT version information in the project
find . -name "CMakeLists.txt" -o -name "setup.py" -o -name "requirements.txt" -o -name ".root-version" | head -5Repository: fcs-proj/FastCaloSim
Length of output: 200
🏁 Script executed:
# Check ROOT version requirement in CMakeLists.txt
grep -i "root\|find_package" ./CMakeLists.txt | head -20Repository: fcs-proj/FastCaloSim
Length of output: 838
🏁 Script executed:
# Search for similar patterns of cd() followed by Get() in the codebase
rg "->cd\(" -A 5 | grep -A 5 "->Get\(" | head -40Repository: fcs-proj/FastCaloSim
Length of output: 579
🏁 Script executed:
# Search for similar patterns of cd() followed by Get() in the codebase
rg '\->cd\(' -A 5 | head -60Repository: fcs-proj/FastCaloSim
Length of output: 5612
Replace inputfile->Get() calls with gDirectory->Get() after directory change.
The code calls inputfile->cd(folder.c_str()) to change the current ROOT directory, but the subsequent inputfile->Get(...) calls on lines 61, 70, 78, 91, and 124 are made directly on the TFile object, which may not respect the current directory context set by cd(). After changing directory, use gDirectory->Get(...) for objects in the current directory, or use TDirectory* dir = inputfile->GetDirectory(folder.c_str()); dir->Get(...) to avoid relying on global state.
The codebase already demonstrates the correct pattern in TFCSPCAEnergyParametrization.cxx, which uses file->cd() followed by gDirectory->Get().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/Core/TFCSVoxelHistoLateralCovarianceFluctuations.cxx` at line 54,
After calling inputfile->cd(folder.c_str()) in
TFCSVoxelHistoLateralCovarianceFluctuations.cxx, replace subsequent uses of
inputfile->Get(...) (the object retrievals performed after the cd) with
gDirectory->Get(...) so the lookup uses the current ROOT directory;
alternatively obtain a TDirectory* with inputfile->GetDirectory(folder.c_str())
and call dir->Get(...) to avoid relying on global state—update the retrievals
that reference inputfile->Get(...) (following the inputfile->cd(folder.c_str())
call) to one of these patterns.
Comment on lines
+123
to
+131
| std::string histname = Form("hist_%d_%d%s", x, y, label.c_str()); | ||
| TH1* hist = (TH1*)inputfile->Get(histname.c_str()); | ||
| if (!hist) { | ||
| FCS_MSG_ERROR("Histogram " << histname << " not found"); | ||
| return false; | ||
| } | ||
| TFCS1DFunctionInt32Int32Histogram* func = | ||
| new TFCS1DFunctionInt32Int32Histogram(); | ||
| func->Initialize(hist, false); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
ROOT TObject TH1 dynamic_cast from TDirectory Get recommended type checking
💡 Result:
In ROOT, when retrieving an object like a TH1 histogram from a TDirectory (e.g., via TDirectory::Get), the method returns a TObject*. To safely cast to TH1*, use dynamic_cast, especially since TH1 inherits from TObject as its first base class. Recommended approaches: 1. Preferred safe method without manual casting: Use TDirectory::GetObject TH1 hist = nullptr; dir->GetObject("histName", hist); if (hist) { // Use hist as TH1 } This performs type checking internally and sets hist only if compatible. 2. Using dynamic_cast on Get result: TObject obj = dir->Get("histName"); TH1 hist = dynamic_cast<TH1>(obj); if (hist) { // Safe to use as TH1 } Official ROOT documentation explicitly recommends dynamic_cast for cases where the object derives from TObject but not as first inheritance (e.g., multiple inheritance), though for TH1 (public TObject, public TArrayD) a C-style cast also works. Dynamic_cast provides runtime type safety and returns nullptr on failure. Avoid unsafe C-style casts like (TH1*)obj without checks, as they can lead to undefined behavior if types mismatch. Example code: TDirectory *dir = ...; TH1 h = dynamic_cast<TH1>(dir->Get("myhist")); if (h) h->Draw; or TH1 *h = nullptr; dir->GetObject("myhist", h); if (h) h->Draw; Both are recommended; GetObject is more concise for known types like TH1.
Citations:
- 1: https://root.cern/doc/v636/classTDirectory.html
- 2: https://root.cern.ch/doc/latest-stable/classTDirectory.html
- 3: https://docs.huihoo.com/root/reference-guide/6.04/TDirectory.html
- 4: https://root.cern/doc/v638/classTDirectory.html
🏁 Script executed:
# Check for similar unsafe C-style casts with TH1 in the same file
grep -n "(TH1\*)" source/Core/TFCSVoxelHistoLateralCovarianceFluctuations.cxxRepository: fcs-proj/FastCaloSim
Length of output: 127
🏁 Script executed:
# Search for other unsafe casts in related files
find source/Core -name "*Histogram*.cxx" -o -name "*Histo*.cxx" | head -20 | while read f; do
echo "=== $f ==="
grep -n "(TH1\*)\|Get(" "$f" 2>/dev/null | head -10
doneRepository: fcs-proj/FastCaloSim
Length of output: 1538
🏁 Script executed:
# Check if dynamic_cast is already used elsewhere in this file
grep -n "dynamic_cast" source/Core/TFCSVoxelHistoLateralCovarianceFluctuations.cxxRepository: fcs-proj/FastCaloSim
Length of output: 238
Use dynamic_cast instead of C-style cast when retrieving TH1 from ROOT file.
Line 124 uses (TH1*)inputfile->Get(...), an unsafe C-style cast that can produce an invalid non-null pointer if the retrieved object is not a TH1. This file already demonstrates the proper pattern elsewhere (lines 60, 88, 96 use dynamic_cast). ROOT documentation explicitly recommends dynamic_cast<TH1*> for runtime type safety when casting TObject* results from Get().
Proposed fix
- TH1* hist = (TH1*)inputfile->Get(histname.c_str());
+ TH1* hist = dynamic_cast<TH1*>(inputfile->Get(histname.c_str()));
if (!hist) {
- FCS_MSG_ERROR("Histogram " << histname << " not found");
+ FCS_MSG_ERROR("Histogram " << histname << " not found or has unexpected type");
return false;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/Core/TFCSVoxelHistoLateralCovarianceFluctuations.cxx` around lines 123
- 131, Replace the unsafe C-style cast when fetching the histogram with a
runtime-checked dynamic_cast: after building histname and calling
inputfile->Get(histname.c_str()), use dynamic_cast<TH1*> on the returned
TObject* and check for nullptr; if the dynamic_cast returns nullptr, log the
same FCS_MSG_ERROR("Histogram " << histname << " not found") and return false,
otherwise pass the safely cast TH1* to TFCS1DFunctionInt32Int32Histogram via
func->Initialize(hist, false).
Comment on lines
+187
to
+189
| int Ebin = simulstate.Ebin(); | ||
|
|
||
| for (unsigned int ilayer = 0; ilayer < m_geo->n_layers(); ++ilayer) { |
There was a problem hiding this comment.
Guard the geometry pointer before using it.
The method’s no-op path handles uninitialized PCA data, but m_geo->n_layers() is dereferenced before that check. If geometry was not set, this crashes before the intended no-op/fatal handling can run.
Proposed fix
int Ebin = simulstate.Ebin();
+ if (!m_geo) {
+ if (m_parMeans.empty() || Ebin <= 0) {
+ return FCSSuccess;
+ }
+ FCS_MSG_ERROR("Geometry is not set");
+ return FCSFatal;
+ }
+
for (unsigned int ilayer = 0; ilayer < m_geo->n_layers(); ++ilayer) {Also applies to: 213-217
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/Core/TFCSVoxelHistoLateralCovarianceFluctuations.cxx` around lines 187
- 189, m_geo is dereferenced before the method's early-exit/no-op handling for
uninitialized PCA runs, causing a crash; add a null-check for m_geo before
calling m_geo->n_layers() (and before any later uses around the block where Ebin
and the for-loop over ilayer are declared) and perform the same
early-return/fatal-handling used for the uninitialized PCA path if m_geo is null
so that the loop using m_geo->n_layers() only executes when m_geo is valid (also
apply the same guard to the subsequent occurrences around the later block
referenced in the comment).
This was referenced May 8, 2026
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.
Summary by CodeRabbit
New Features
Refactor
Chores