Below a report generated by Sol (Medium) using:

Do a deep review of the Eventdisplay path of V2DL3 - start with the documentation and understand its typical usage. Look also at the general tools.
Ignore anything related to VEGAS. Find bugs and inconsistencies. Write a report in markdown about your findings. Make suggestions on how to improve
the code.

Some of finding are in side branches hardly (never used), but some definitely requires some attention.

Review of the Eventdisplay path in V2DL3

Review date: 2026-08-04
Reviewed revision: 448b2a3 (eventdisplay-maintenance, identical to main at review time)
Scope: Eventdisplay conversion, shared FITS writers, index generation, comparison/query utilities, documentation, packaging, tests, and Eventdisplay CI. VEGAS-specific code and documentation were excluded.

Executive summary

The normal Eventdisplay conversion path is compact and understandable: an anasum ROOT file and an effective-area ROOT file are converted into one DL3 FITS file, after which a separate command creates observation and HDU index tables. The implementation has useful separation between Eventdisplay ROOT extraction and shared FITS serialization, supports point-like and full-enclosure responses, and has a reproducible integration fixture.

The baseline is nevertheless unsafe for scientific production without fixes. The most important issue is that all generated IRF offset bins have zero width (THETA_LO == THETA_HI). This affects effective area and energy dispersion for both modes and the PSF for full-enclosure mode. The checked-in reference products contain the same defect, so the current exact-output CI comparison reports success. Other high-impact findings include incorrect azimuth-bin selection around the 0/360-degree boundary, event filters changing the run parameters used for IRF interpolation, misleading extrapolation semantics with the default K-nearest-neighbor interpolator, broken filename-derived observation IDs, and silent truncation of paths in HDU index files.

The repository's 31 unit tests pass, and newly generated baseline point-like and full-enclosure products match the stored references. Those facts establish reproducibility, not format or scientific correctness: the tests do not exercise most command-line behavior or semantic GADF validation.

Typical documented workflow

The README describes the following Eventdisplay workflow:

1. Install with pip install v2dl3, or create v2dl3Eventdisplay from environment-eventdisplay.yml for development.
2. Convert one run with v2dl3-eventdisplay -f RUN.anasum.root EFFECTIVE_AREA.root OUTPUT.fits.
3. Select --full-enclosure when needed; point-like output is the default.
4. Optionally select an interpolator, allow extrapolation, clamp near an IRF boundary, add telescope multiplicity or database metadata, and filter events with YAML/JSON.
5. Run v2dl3-generate-index-file over the resulting per-observation files to create obs-index.fits.gz and hdu-index.fits.gz for Gammapy.

The implementation follows this broad flow:

• pyV2DL3/script/v2dl3_for_Eventdisplay.py defines the CLI.
• EventDisplayDataSource orchestrates event/GTI extraction and response generation.
• eventdisplay/fillEVENTS.py reads Eventdisplay trees, creates event metadata, and decodes the time mask.
• IrfExtractor, IrfInterpolator, and eventdisplay/fillRESPONSE.py select and interpolate IRFs.
• Shared fillEVENTS.py, fillGTI.py, and fillRESPONSE.py serialize FITS extensions.
• generateObsHduIndex.py and script/generate_index_file.py build the data-store indices.

Findings

ED-01 — Critical: generated response offset bins all have zero width

find_camera_offsets() returns the same camera-offset array for both lower and upper edges (eventdisplay/fillRESPONSE.py:143-162). These arrays are written directly as THETA_LO and THETA_HI in effective area, energy dispersion, and PSF tables (eventdisplay/fillRESPONSE.py:198-216, 231-246, and 332-345).

This was reproduced with the bundled run and effective-area data. Every response extension in both point-like and full-enclosure outputs had:

THETA_LO = [0.00, 0.25, ..., 2.00]
THETA_HI = [0.00, 0.25, ..., 2.00]
all(THETA_HI > THETA_LO) = False

The GADF 0.3 effective-area, energy-dispersion, and PSF table definitions specify THETA_LO/THETA_HI as the field-of-view offset axis: effective area, energy dispersion, and PSF table. Equal lower and upper bounds describe empty bins and can break or distort downstream axis lookup and integration.

The stored CI reference files have the same values, so byte/value comparison cannot catch this.

Suggested fix:

• Convert simulated offset centers into real edges using adjacent midpoints, with an explicitly chosen boundary policy at zero and at the outer camera edge.
• Keep interpolation coordinates separate from serialized bin edges. A duplicated interpolation node must not become two zero-width FITS bins.
• Add semantic tests asserting finite, ordered, non-overlapping axes and HI > LO for every response axis.
• Load the output with a supported Gammapy release and evaluate/interpolate each IRF at interior and boundary coordinates.

ED-02 — High: azimuth selection is not circular at north

• fixed in Eventdisplay converter: azimuth selection is not circular at north #260

find_closest_az() converts negative bin centers to [0, 360], then calls find_nearest(), which minimizes ordinary absolute distance (IrfExtractor.py:24-26 and 44-63). It does not use circular angular distance.

For the bundled effective-area binning, an observation azimuth of 359 degrees selects index 7 (the 345-degree bin), even though the 7.5-degree bin is closer across the wrap boundary. This can select the wrong IRF slice for observations close to north.

Suggested fix: compute abs(((center - azimuth + 180) % 360) - 180) and add regression cases around 0/360, exact boundaries, and negative input normalization.

ED-03 — High: event filtering can change the IRF applied to the observation

The event mask is applied before calculating mean pedestal variance, mean event altitude/azimuth, and the telescope mask (eventdisplay/fillEVENTS.py:100-126). Those masked values then define NSBLEVEL, ALT_PNT, AZ_PNT, and the zenith/pedestal/azimuth coordinates passed to IRF interpolation (eventdisplay/fillEVENTS.py:53-68 and 73-85).

This couples a user-level event selection to the instrument response. An energy, gammaness, or spatial filter can therefore change the IRF attached to the same observation. In the bundled sample, selecting 1–10 TeV changed the pedestal variance from about 7.2970 to 7.2952; the difference is small for that run but proves the coupling.

It also labels averages of reconstructed event directions as ALT_PNT/AZ_PNT, while RA/Dec pointing comes from pointingDataReduced. This is conceptually inconsistent and makes the header dependent on the event population.

Suggested fix:

• Derive IRF coordinates and pointing metadata from unfiltered run/pointing information.
• Apply evt_filter only to columns written to the EVENTS table.
• Calculate run-level pedestal variance from an unfiltered source, or document and validate the intended source explicitly.
• Test that adding a non-time event filter leaves all IRF arrays and observation-level headers unchanged.

ED-04 — High: --filename_to_obsid creates invalid and inconsistent observation IDs

The option derives the ID with one os.path.splitext() call and writes it only to the EVENTS header (v2dl3_for_Eventdisplay.py:160-166). For /tmp/custom-name.fits.gz, this produces the string custom-name.fits, not an integer. The logging call formats that string with %d, producing a runtime logging traceback. The response extensions retain the original integer run number.

Reproduced output:

EVENTS OBS_ID = 'custom-name.fits'
EFFECTIVE AREA OBS_ID = 64080
ENERGY DISPERSION OBS_ID = 64080

GADF defines the EVENTS OBS_ID as an integer unique observation identifier (EVENTS specification). The index writer also declares OBS_ID as >i8, so arbitrary strings are incompatible with the next documented step.

Suggested fix: replace this option with an explicit integer --obs-id, validate it with Click, update the data source before generating any HDUs, and ensure every applicable extension and both indices use the same value. If filename inference is retained, strip all recognized FITS suffixes and require the remaining stem to parse as an integer.

ED-05 — High: HDU index paths are silently truncated

gen_hdu_index() fixes FILE_DIR to S40 and FILE_NAME to S54 (generateObsHduIndex.py:63-78). Astropy silently truncated a 60-character directory to 40 characters in a targeted reproduction. The resulting index can point to a nonexistent file while remaining a syntactically valid FITS table.

Suggested fix: determine widths from the maximum encoded values in the completed table, impose and validate any intentional format limit, and round-trip every indexed path in an integration test.

ED-06 — Medium: both IRF mode flags are accepted and point-like silently wins

--point-like and --full-enclosure are independent flags (v2dl3_for_Eventdisplay.py:31-40). When both are true, response generation and serialization use if point-like ... elif full-enclosure, so the result contains only point-like IRFs (eventdisplay/fillRESPONSE.py:375-447; shared fillRESPONSE.py:95-173).

This was reproduced: the log reports both modes enabled, while the FITS file contains only point-like effective area and energy dispersion.

Suggested fix: make the modes a single click.Choice option or reject the conflicting combination with click.UsageError.

ED-07 — Medium: the documented energy-filter example fails

The README uses:

ENERGY: [1, 10]

The filter is evaluated against raw Eventdisplay branch names before columns are renamed (eventdisplay/fillEVENTS.py:259-282). The actual branch is Energy, so the documented example raises KeyError: 'ENERGY'; Energy: [1, 10] works.

Suggested fix: define a stable public filter vocabulary matching output column names, translate it to ROOT branches internally, and validate unknown keys with a message listing supported names. At minimum, correct the README.

ED-08 — Medium: valid filters that select zero events crash with an unrelated NumPy error

The empty-list check runs before applying the mask (eventdisplay/fillEVENTS.py:95-100). After filtering, np.max(ImgSel[mask]) is called unconditionally (eventdisplay/fillEVENTS.py:123-126). A filter selecting no events therefore raises:

ValueError: zero-size array to reduction operation maximum which has no identity

Malformed range lists also fail with an unhelpful IndexError because list length is not checked (eventdisplay/fillEVENTS.py:269-274).

Suggested fix: validate filter schema and keys before evaluation, check the post-filter event count, and raise a domain-specific Click error identifying the filter and input file.

ED-09 — Medium: all-masked GTIs crash; missing time-mask fallback is fragile

getGTI() strips trailing zero bits and then indexes time_array_sec[0] and [-1] without handling an empty string (eventdisplay/util.py:82-119). An all-zero two-byte mask reproduces IndexError: string index out of range. Line 118 also compares a one-character string to integer zero, so the condition is always true when reached.

The missing-mask fallback catches KeyError, then immediately iterates through the same timeMask path (eventdisplay/fillEVENTS.py:226-236). If the whole timeMask node is absent rather than only maskBits, the fallback raises a second KeyError instead of using the run start/stop interval.

Suggested fix: decode bits into a Boolean NumPy array, explicitly handle all-good, all-bad, and empty masks, and make the fallback independent of the missing object. Decide whether an all-bad run should produce an empty GTI or fail conversion with a clear quality message.

ED-10 — Medium: --force_extrapolation does not provide the documented linear extrapolation with the default interpolator

The README states that this option linearly extrapolates the IRF. check_parameter_range() merely allows the out-of-range coordinate (eventdisplay/fillRESPONSE.py:53-100). With RegularGridInterpolator, fill_value=None does request linear extrapolation (IrfInterpolator.py:135-145). With the default KNeighborsRegressor, prediction remains a distance-weighted nearest-neighbor estimate (IrfInterpolator.py:75-92) and is not linear extrapolation.

This is a scientific semantics mismatch: the same CLI flag means different mathematics depending on another option.

Suggested fix: disallow forced extrapolation for KNN, implement and test an explicit extrapolator, or accurately document the method-specific behavior. Log the chosen algorithm and out-of-bounds distance in every extrapolated output.

ED-11 — Medium: zero-degree azimuth is treated as missing in the regular-grid path

extract_irf() uses if not azimuth, rejecting the valid value 0.0 (IrfExtractor.py:201-212). IrfInterpolator.interpolate() also has a special self.azimuth == 0 branch requiring four coordinates although all callers provide [pedvar, zenith, offset] (IrfInterpolator.py:148-164; eventdisplay/fillRESPONSE.py:191, 223, and 267).

The default KNN extractor bypasses the first check, but the coordinate-length check still makes an exactly zero-degree run fail. The regular-grid implementation fails earlier.

Suggested fix: test azimuth is None, remove the unexplained coordinate-count special case, normalize azimuth consistently, and add exact 0/360 tests for both interpolators.

ED-12 — Medium: index CLI existence checks ignore custom output names

When --recreate is absent, generate_index_file.py:111-119 checks only hard-coded obs-index.fits.gz and hdu-index.fits.gz, not the values supplied through --obs_index_file and --hdu_index_file.

Consequences:

• Existing default-named files prevent creation even when different custom names were requested.
• Existing custom-named files are not detected and are overwritten because the lower-level writer always uses overwrite=True.

The converter itself also always overwrites its output (v2dl3_for_Eventdisplay.py:167) without an explicit --overwrite option.

Suggested fix: construct exact output Path objects once, check those paths, and require explicit overwrite authorization.

ED-13 — Medium: database metadata is neither associated with nor isolated from the run

read_db_fits_file() always reads row zero from the DQM table and does not verify its observation/run identifier (DBFitsFile.py:24-29). eventdisplay/fillEVENTS.py:71 then applies an unrestricted evt_dict.update(). A multi-row or wrong-run file can silently attach unrelated metadata, and colliding DQM column names can overwrite core values such as OBS_ID, timing, or pointing.

Suggested fix: require and match an observation ID, reject zero or multiple matches, whitelist supported auxiliary columns, and prevent database values from replacing core conversion fields.

ED-14 — Medium: reusable Eventdisplay code depends implicitly on an active Click context

Range checks, interpolation setup, and extrapolation read CLI parameters through click.get_current_context() by default (eventdisplay/fillRESPONSE.py:69-75 and 355-359; IrfInterpolator.py:135-139). Calling the otherwise public-looking sequence loadROOTFiles(..., "Eventdisplay"); datasource.fill_data() outside the CLI reproduces RuntimeError: There is no active click context.

There is a hidden use_click=False path, but it is not a clean or documented API and option shapes differ (for example fuzzy-boundary handling).

Suggested fix: create a typed conversion configuration object in the CLI and pass it through the data-source/IRF layers. Domain code should not import Click or inspect global command context.

ED-15 — Low: version/query code relies on private Uproot internals and unchecked log text

Both EventDisplayDataSource._fill_data_source_version() and eventdisplay_query_runparameters.py access members['fLines']._data, where _data is private Uproot state. The version path checks that the log exists but not that the expected version line exists, so [...][0] can raise IndexError (EventDisplayDataSource.py:87-95). The query helper similarly assumes exact log phrases and split delimiters (eventdisplay_query_runparameters.py:14-29) and does not use a context manager.

Suggested fix: isolate ROOT log decoding behind one tested adapter, use public Uproot APIs where possible, handle absent/multiple matches explicitly, and include file/run context in errors.

ED-16 — Low: comparison CLI reports differences with a successful exit status

v2dl3-compareFitsFiles writes FITSDiff.report() but never checks fd.identical or raises on differences (script/compareFitsFiles.py:23-30). CI compensates by grepping the report text. Other automation can receive exit code zero for unequal files.

Suggested fix: print/write the report and exit nonzero when files differ. Add options for tolerances and ignored keywords, and test both equal and unequal cases.

ED-17 — Low: documentation and CLI text contain smaller inconsistencies

• README says --save_multiplicity writes an EVENTTYPE keyword; code writes an EVENT_TYPE table column (README.md:146; shared fillEVENTS.py:66-70).
• The CLI help says “filter events form json or yaml” and the fuzzy-boundary help concatenates words because escaped source lines omit spaces (v2dl3_for_Eventdisplay.py:64-87).
• The README documents a tolerance argument as if singular, although the CLI expects repeated axis/value pairs.
• The index section links GADF v0.2 while files declare HDUVERS=0.3 (README.md:175-177; constant.py:12-15).
• environment-eventdisplay.yml allows any Python >=3.11, while CI tests only 3.13 and package metadata claims Python 3.8 onward. The review environment resolved Python 3.14, which is not represented by the classifiers or CI.

Suggested fix: generate CLI examples from tested invocations, align supported Python constraints/classifiers/CI, and add a short Eventdisplay-specific reference page describing input tree expectations, response-mode semantics, filter names, output naming, and failure behavior.

Cross-cutting design and quality recommendations

1. Add semantic DL3 validation, not only reference comparison

The current integration tests prove deterministic reproduction of stored output, including stored defects. Add a validator stage that independently checks:

• required extensions, columns, units, HDU class keywords, and integer observation IDs;
• consistent OBS_ID, time reference, telescope, and instrument metadata across extensions;
• strictly increasing axes with finite values, HI > LO, compatible shapes, and no overlaps/gaps beyond an explicit policy;
• non-negative effective area, normalized energy dispersion, and approximately normalized PSF;
• event times within observation/GTI bounds and monotonic ordering where required;
• successful loading and evaluation through the minimum and latest supported Gammapy versions;
• successful index round-trip from each FILE_DIR/FILE_NAME entry.

Keep reference comparisons as regression tests, but regenerate references only after semantic checks pass and require review of a summarized scientific diff.

2. Separate run metadata, event selection, and response construction

Create immutable models such as ObservationMetadata, EventTable, GTI, IrfQuery, and ConversionOptions. Extract observation metadata once from unfiltered run-level inputs, derive IRFs from that metadata, and apply event filters only to EventTable. This removes the current hidden data flow through mutable dictionaries and double-underscore attributes.

3. Validate at system boundaries

Use Click types/callbacks and explicit schema validation to reject:

• conflicting response modes;
• invalid/negative fuzzy tolerances;
• unsupported filter keys, malformed ranges, and empty selections;
• non-integer observation IDs;
• wrong-run or multi-row DQM inputs;
• IRF files lacking required branches, axes, or sufficient parameter coverage;
• output collisions unless overwrite is explicitly requested.

Errors should identify the file, run, branch/axis, supplied value, allowed range, and suggested correction.

4. Make interpolation behavior explicit and testable

Remove Click access from interpolation classes. Give each interpolation/extrapolation method a documented mathematical contract. Test exact grid nodes, interior points, boundaries, circular azimuth selection, single-dimension grids, empty bins, 0/360 degrees, fuzzy clamping, and out-of-range behavior. Record the method and query coordinates in reproducible provenance metadata or logs.

5. Strengthen the test matrix

Add unit and CLI tests for every finding above. Run them across the actually supported Python range. The Eventdisplay workflow currently ignores README.md changes in its pull-request path filter (.github/workflows/v2dl3-eventdisplay.yml:5-15), even though examples are part of the user interface; documentation changes should run at least CLI/example validation.

6. Make index generation transactional

Build and validate both tables in memory, write temporary files in the target directory, then atomically replace both final outputs only after both succeed. Validate duplicate observation IDs, metadata consistency, and path round-trips. Derive string widths from data rather than hard-coding them.

Verification performed

• Read the Eventdisplay README workflow, actual --help, package/environment metadata, Eventdisplay modules, shared FITS writers, general utilities, tests, and Eventdisplay GitHub Actions workflow.
• Excluded all VEGAS implementation and VEGAS-only utilities from findings.
• Ran pytest -q in the v2dl3Eventdisplay environment: 31 passed.
• Ran focused Flake8 fatal-error checks (E9, F63, F7, F82): no findings.
• Generated point-like and full-enclosure files from the bundled 64080 fixtures.
• Compared both outputs to stored reference FITS files: FITSDiff reported no differences.
• Ran Astropy FITS structural verification: no structural FITS errors.
• Independently checked response axis bounds and exposed the zero-width offset axes.
• Checked full-enclosure PSF values: finite, with numerical solid-angle integrals ranging roughly from 0.92 to 1.02 for the sample (useful as a baseline, but not a replacement for downstream-tool validation).
• Reproduced the azimuth wrap, filename observation-ID, README filter, zero-event filter, all-zero GTI, no-Click-context, conflicting-mode, and fixed-width path issues described above.

Recommended implementation order

1. Fix IRF offset bin edges and add semantic/Gammapy validation before releasing more reference products.
2. Decouple filtering from observation metadata and IRF coordinates.
3. Fix circular azimuth handling and exact-zero behavior for both interpolators.
4. Replace filename-derived IDs with a validated integer observation-ID option and enforce cross-HDU consistency.
5. Correct KNN extrapolation semantics or disable the unsupported combination.
6. Harden filter/GTI edge cases and database association.
7. Fix index path widths, custom-output checks, and transactional writes.
8. Expand CLI/integration tests and correct the documentation.