Skip to content

Add data quality-related subgroup definitions to HDF5 data format#2965

Merged
maxnoe merged 3 commits into
mainfrom
update_monitoring_data_format
Mar 19, 2026
Merged

Add data quality-related subgroup definitions to HDF5 data format#2965
maxnoe merged 3 commits into
mainfrom
update_monitoring_data_format

Conversation

@mexanick
Copy link
Copy Markdown
Contributor

@mexanick mexanick commented Mar 6, 2026

No description provided.

@ctao-sonarqube
Copy link
Copy Markdown

ctao-sonarqube Bot commented Mar 6, 2026

@mexanick mexanick marked this pull request as ready for review March 6, 2026 13:21
@mexanick mexanick requested review from juanpala1997 and maxnoe March 6, 2026 13:21
Copy link
Copy Markdown

@juanpala1997 juanpala1997 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proposed changes look okay for me.

I noticed that the structure of both _append_quality_telescope_groups and _append_quality_subarray_groups in hdf5merger.py is very similar, differing only in the monitoring group being traversed. I was wondering if this could be further reduced using only one method with an additional input parameter. But I understand that the current structure probably makes the readability of the code easier, so it seems fine to leave it like this. I approve it.

if self.telescope_events:
self._append_monitoring_telescope_groups(other)
self._append_pixel_statistics(other)
self._append_quality_telescope_groups(other)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we want to tie telescope quality monitoring to telescope events.

Data quality by telescope might still be interesting even if dropping by-telescope DL1 or DL2 reconstructed quantities, right? And data volume is much smaller.

Should we gate this behind a dedicated option? Or just always append if present?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking similarly and decided for the moment to follow the suite of the monitoring data. Adding another option would make an already option-heavy config even larger. On the other hand, from the practical point of view, I think at the merging stage we will always have at least camera calibration events, but also muons. illuminator, etc.

Copy link
Copy Markdown
Member

@maxnoe maxnoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small question inline

@mexanick mexanick added this to the v0.30.0 milestone Mar 11, 2026
@maxnoe maxnoe changed the title Add data quality-related subgroup definitions Add data quality-related subgroup definitions to HDF5 data format Mar 19, 2026
@maxnoe maxnoe merged commit 5d985bc into main Mar 19, 2026
20 of 22 checks passed
@maxnoe maxnoe deleted the update_monitoring_data_format branch March 19, 2026 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants