Codex Code Review

[GernotMaier]

Below a quick review from GTP-5.4.

Code Review Findings

Scope: review of the DL2 and DL3-IRFs converters, focused on major physics and computing bugs that can corrupt exported science products. I did not flag style issues or minor schema nits.

Validation status:

• DL2 was reviewed statically only.
• DL3-IRFs was checked against the example ROOT file DESY.g20260630.V3.ID0_0degNIM2LST2MST3SST3SCMST2.prod6-Paranal-60deg-dark-sq20-LL.S.Am-2LSTs14MSTs37SSTs.18000s.root.
• Using a local ROOT container, I verified that this file contains the histogram objects the converter expects:
  EffectiveAreaEtrueNoTheta2cut_offaxis (TH2F),
  AngResEtrue_offaxis (TH2F),
  EestOverEtrueNoTheta2cut_offaxis (TH3F),
  BGRatePerSqDeg_offaxis (TH2F),
  DiffSens_offaxis (TH2F),
  AngularPSF2DEtrue_offaxis (TH3F).
• I also verified representative axis ranges used below:
  AngResEtrue_offaxis is binned in log10 energy and off-axis angle (x: -1.9..2.3, y: 0..6 deg);
  BGRatePerSqDeg_offaxis is also y: 0..6 deg;
  EestOverEtrueNoTheta2cut_offaxis uses x: log10(E_true) = -2..4, y: migra = 0..3, z: theta = 0..6 deg;
  AngularPSF2DEtrue_offaxis uses x: log10(E_true) = -1.9..3.1, y: radial offset = 0..4.5 deg, z: theta = 0..6 deg.

Findings

1. Multi-file DL2 exports mix event rows from all inputs with MC metadata from only the first file

Severity: High

Files:

• DL2/generate_DL2_file.py
• DL2/generate_DL2_file.py
• DL2/generate_DL2_file.py
• DL2/generate_DL2_file.py

Why this is a bug:

• Event rows are merged across all filenames via uproot.lazy([{f: "DL2EventTree"} for f in filenames], ...).
• The simulated-event histogram hEmcUW, the MC run header, and the site altitude are read only from filenames[0].
• The output FITS therefore combines events from multiple runs/files, but keeps only the first file's simulation histogram and run metadata.

Impact:

• The SIMULATED EVENTS HDU becomes wrong as soon as more than one input file is passed.
• Any downstream efficiency, exposure, or selection-efficiency calculation using that HDU will be biased, typically undercounting or overcounting MC statistics depending on how many files were merged.
• The RUNHEADER and ALTITUDE metadata can also describe the wrong run configuration for a subset of exported events.

Recommended fix:

• Either forbid multi-file export for now and fail fast, or aggregate hEmcUW, run-header fields, and site metadata consistently across all input files.
• If multiple files are meant to represent distinct observations/configurations, the output should probably split them instead of flattening them into a single shared metadata block.

2. Event-type filtering rescales the MC histogram with one global ratio instead of an energy-dependent correction

Severity: High

Files:

• DL2/generate_DL2_file.py
• DL2/generate_DL2_file.py
• DL2/generate_DL2_file.py

Why this is a bug:

• When event_type > 0, training events are removed from the exported event list by masking individual events.
• The MC histogram is then corrected with num_events = bin_content * (1 - ratio), where ratio is one global fraction of event_types == -1.
• This assumes the removed training sample is uniformly distributed in energy, which is almost never guaranteed for classifier training samples.

Impact:

• The SIMULATED EVENTS HDU gets the wrong energy spectrum whenever the withheld/training events are energy dependent.
• Any later use of that HDU to derive effective areas or normalize IRFs will inherit an energy-dependent bias.
• This is especially risky because the bug is silent: the output still looks structurally valid.

Recommended fix:

• Rebuild the simulated-event histogram from event-wise MC energies after applying the actual event-type selection, or subtract training events per MC-energy bin.
• If the sidecar .txt file does not cover all events needed to do that, the format is insufficient for correct DL2 export and should be extended.

3. The PSF SCALE term is computed with the wrong formula and wrong units

Severity: High

Files:

• DL3-IRFs/src/convertSensitivityFilesToFITS.cpp
• DL3-IRFs/src/VDL3IRFs.cpp
• DL3-IRFs/src/VDL3IRFs.cpp

Why this is a bug:

• write_psf_gauss() exports a PSF in PSF_3GAUSS format and labels SCALE with units sr^(-1).
• The code computes SCALE = 1 / (2 * pi * sigma).
• For a 2D Gaussian, the normalization should scale like 1 / (2 * pi * sigma^2), and since the FITS unit is sr^-1, sigma must be converted to radians before normalization.
• The example ROOT file confirms that the input AngResEtrue_offaxis histogram is a 68% containment radius map, not a pre-normalized Gaussian amplitude map, so this normalization step is definitely active on real data.

Impact:

• The exported PSF normalization is dimensionally inconsistent and numerically wrong by orders of magnitude.
• Any tool that uses the SCALE parameter rather than treating the file as a loose containment-radius proxy will get an incorrect PSF amplitude and therefore incorrect source morphology or likelihood values.

Recommended fix:

• Decide on the exact PSF_3GAUSS parameterization required by the target GADF version, then compute SCALE from sigma in radians with the correct squared dependence.
• Add a regression test that numerically integrates the exported PSF and checks that it normalizes to unity.

4. The IRF converter ignores write failures and can exit successfully after producing a partial or invalid FITS file

Severity: High

Files:

• DL3-IRFs/src/convertSensitivityFilesToFITS.cpp
• DL3-IRFs/src/convertSensitivityFilesToFITS.cpp
• DL3-IRFs/src/convertSensitivityFilesToFITS.cpp
• DL3-IRFs/src/convertSensitivityFilesToFITS.cpp
• DL3-IRFs/src/convertSensitivityFilesToFITS.cpp
• DL3-IRFs/src/convertSensitivityFilesToFITS.cpp

Why this is a bug:

• All write_*() methods return bool, and many return false if a histogram is missing or a CFITSIO call fails.
• main() ignores every one of those return values and still closes the file and returns 0.

Impact:

• A missing histogram, wrong ROOT object name, or FITS write failure can silently produce a truncated output file with missing IRF HDUs.
• In production pipelines this is dangerous because the converter reports success even when the science product is incomplete.
• This is a computing bug with direct science consequences: downstream analyses may use an invalid IRF file without noticing until much later.
• For the provided example ROOT file specifically, the expected histogram names do exist, so this failure mode is not triggered by missing objects in this sample. The bug still matters because the converter has no protection against future format drift or partial inputs.

Recommended fix:

• Check every write_*() result and abort on failure with a non-zero exit status.
• Validate that all required input histograms exist before creating the output file.
• Consider running the schema validator as part of the conversion path, not only as a separate manual test.

Residual Risk / Gaps

• I verified the structure of one real DL3 ROOT input, but I did not complete an end-to-end FITS conversion and schema validation in this environment, so there may still be additional data-dependent issues in the written FITS layout.
• The PSF table path (write_psf_table) is currently disabled in main(), so I did not include its schema/physics issues in the main findings list.