Skip to content

Adding calibration dataformat constants - #3053

Open
juanpala1997 wants to merge 2 commits into
mainfrom
add-calibration-dataformat-constants
Open

Adding calibration dataformat constants#3053
juanpala1997 wants to merge 2 commits into
mainfrom
add-calibration-dataformat-constants

Conversation

@juanpala1997

Copy link
Copy Markdown

This PR closes #3052

It adds three new H5DF data format constants to ctapipe.io.hdf5dataformat

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Adds missing HDF5 data format constants needed by DPPS CalibPipe (per #3052) to ctapipe.io.hdf5dataformat, extending the set of standardized group paths used across ctapipe’s HDF5 I/O.

Changes:

  • Added DL2_TEL_IMPACT_GROUP for "/dl2/event/telescope/impact".
  • Added DL1_MONITORING_ATMOSPHERE and DL1_MONITORING_CLEAR_NIGHT for atmospheric monitoring paths.
  • Exported the new constants via __all__.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@ctao-sonarqube

ctao-sonarqube Bot commented Jul 2, 2026

Copy link
Copy Markdown

Comment thread src/ctapipe/io/hdf5dataformat.py
Comment thread src/ctapipe/io/hdf5dataformat.py Outdated
"DL1_FLATFIELD_IMAGE_GROUP",
"DL1_FLATFIELD_PEAK_TIME_GROUP",
"DL1_MONITORING_ATMOSPHERE",
"DL1_MONITORING_CLEAR_NIGHT",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What kind of data would be stored there? Is it format unique for clear nights?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No, not unique. I agree that the naming (clear night) should be changed. The data to be stored there will be the assessment of the night. In the table structure, a column will be the night assessment, i.e. clear, night, Calima, single Cirrus layer etc.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

could you provide a provisional schema for the table?

@Voutsi Voutsi Jul 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Table schema:
name, type, units
night_assessment String None
VAOD float64 None
fractional_rms_variability_amplitude float64 None

"DL1_SKY_PEDESTAL_IMAGE_GROUP",
"DL1_FLATFIELD_IMAGE_GROUP",
"DL1_FLATFIELD_PEAK_TIME_GROUP",
"DL1_MONITORING_ATMOSPHERE",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What's the plan of interfacing of this with existing atmospheric module?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I don't understand your comment. Do you mean the connection with ATMOSPHERE_DENSITY_PROFILE_TABLE?
Those are very different things. The point of adding the DL1_MONITORING_ATMOSPHERE group is to write down information monitoring the quality of the atmosphere. I think this is information is useful in order to monitor long term trends.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I got an impression you were planning also to store the contemporary VAOD profiles, and I would naively assume that have to be eventually combined with the density profiles in order to compute the total attenuation of the Cherenkov light.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I see. Let me clarify. I do plan to store the VAOD. However the VAOD is not a profile, it is a single value expressing the optical depth at zenith from the ground to the top of the atmosphere.

I would naively assume that have to be eventually combined with the density profiles in order to compute the total attenuation of the Cherenkov light.

Indeed one has to combine the Rayleigh scattering profile (not density) with molecular absorption and aerosol profiles. But this is outside of the scope of this PR.

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.

the atmosphere density profile is Service data, and this is Monitoring, so it think it's clear that they are different.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hi @kosack , I am wondering if the atmosphere density profile should be service data. CalibPipe will produce density profiles to be used for tailored simulations. That should be then monitoring data, right?

@kosack kosack Jul 30, 2026

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.

Monitoring data is something that changes in time during an observation, i.e. it is a table with a TIMESTAMP column, and values that are interpolated to the event time.

An atmosphere profile is at least static for a given day. or obs_id, I guess, right? You would not expect to have to interpolate it to an event time. If so, it should still be Service data. But if you plan to write out a time-series of these for a given night or something like that, then it could be Monitoring. What is the plan?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hi @kosack , an atmospheric density model will be static for a day or OB (I am not sure if that's somewhere documented but I don't think it makes sense to produce density models in time intervals shorter than an OB, the density changes slowly).

On the other hand, we might produce more than one extinction profile within one OB, when estimating good time intervals.

You would not expect to have to interpolate it to an event time. If so, it should still be Service data.

That makes me think that the inter - cross calibration coefficients should go to service data as well...

@maxnoe

maxnoe commented Jul 2, 2026

Copy link
Copy Markdown
Member

I don't understand why a group would be named clear_night.

I would expect an assessment of the atmosphere to be stored in some metrics inside of a dataset, not in a group name.

@Voutsi

Voutsi commented Jul 5, 2026

Copy link
Copy Markdown

I don't understand why a group would be named clear_night.

I would expect an assessment of the atmosphere to be stored in some metrics inside of a dataset, not in a group name.

I see your point. As I wrote above, the naming I chose for the group is unfortunate. I would suggest to remove then the DL1_MONITORING_CLEAR_NIGHT group and write the assessment of the night inside a table under DL1_MONITORING_ATMOSPHERE

@juanpala1997
juanpala1997 requested review from maxnoe and removed request for Voutsi July 6, 2026 16:26
@mexanick

mexanick commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

In case a group (like DL1_MONITORING_ATMOSPHERE) will have multiple tables that aren't separated by the telescopes (or other array element ID), the table paths should be also prescribed in separate constants, like e.g.

ATMOSPHERE_DENSITY_PROFILE_TABLE = "/simulation/service/atmosphere_density_profile"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add calibration-related HDF5 data format constants

6 participants