Skip to content

add MonitoringSource#2802

Merged
maxnoe merged 94 commits into
mainfrom
calibpipe_interface
Oct 7, 2025
Merged

add MonitoringSource#2802
maxnoe merged 94 commits into
mainfrom
calibpipe_interface

Conversation

@TjarkMiener
Copy link
Copy Markdown
Member

@TjarkMiener TjarkMiener commented Jul 25, 2025

This PR prepares ctapipe to interface with camera calibration data products from calibpipe via a new MonitoringSource().

Fixes #2631
Fixes #2565
Fixes #1870
Fixes #1397
Fixes #1164
Fixes #1041
Fixes #856
Fixes #747

@TjarkMiener TjarkMiener requested a review from mexanick July 25, 2025 14:41
@TjarkMiener TjarkMiener self-assigned this Jul 25, 2025
@ctao-dpps-sonarqube

This comment has been minimized.

Comment thread docs/changes/2802.maintenance.rst Outdated
Comment thread src/ctapipe/containers.py Outdated
Comment thread src/ctapipe/calib/camera/calibrator.py Outdated
@ctao-dpps-sonarqube

This comment has been minimized.

@ctao-dpps-sonarqube

This comment has been minimized.

@TjarkMiener
Copy link
Copy Markdown
Member Author

Coverage is low as expected because we do not have a monitoring file running in the CI and proper unit tests.

@ctao-dpps-sonarqube

This comment has been minimized.

@TjarkMiener TjarkMiener force-pushed the calibpipe_interface branch from 1af6afd to 9ce1c90 Compare August 1, 2025 10:05
@ctao-dpps-sonarqube

This comment has been minimized.

Comment thread src/ctapipe/io/simteleventsource.py Outdated
@ctao-dpps-sonarqube

This comment has been minimized.

@ctao-dpps-sonarqube

This comment has been minimized.

@TjarkMiener TjarkMiener requested a review from mexanick August 12, 2025 15:45
@ctao-dpps-sonarqube

This comment has been minimized.

Comment thread src/ctapipe/io/simteleventsource.py Outdated
Comment thread src/ctapipe/io/simteleventsource.py Outdated
Comment thread src/ctapipe/io/tests/test_simteleventsource.py Outdated
@mexanick mexanick self-requested a review August 13, 2025 09:47
mexanick
mexanick previously approved these changes Aug 13, 2025
@TjarkMiener TjarkMiener marked this pull request as ready for review August 13, 2025 09:54
@TjarkMiener TjarkMiener requested review from kosack and maxnoe August 13, 2025 09:55
@ctao-dpps-sonarqube

This comment has been minimized.

Comment thread src/ctapipe/calib/camera/calibrator.py
Comment thread src/ctapipe/io/simteleventsource.py Outdated
Comment thread src/ctapipe/io/simteleventsource.py Outdated
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.

As commented inline, I think this must be implemented independently of any particular event source implementation with a new type of source.

@TjarkMiener
Copy link
Copy Markdown
Member Author

Thanks for the valuable suggestion, @maxnoe! I wonder if this MonitoringSource can be a child from the HDF5EventSource rather than be an independent component, since we are using the merger component to create the monitoring file. It might just be a matter of overwritting the _generator() in the MonitoringSource child to suit our use case. What do you think?

@maxnoe
Copy link
Copy Markdown
Member

maxnoe commented Aug 15, 2025

A child? As in the MonitoringSource is an EventSource? No, that's exactly what I want to avoid.

I want to introduce the new concept of something that attaches information by timestamp to an existing event, not creates new events.

This has been discussed a couple of times in issues and no is the right time to implement it:

#1041

@TjarkMiener TjarkMiener marked this pull request as draft August 18, 2025 08:53
@TjarkMiener TjarkMiener changed the title Calibpipe interface add MonitoringSource Aug 18, 2025
Comment thread docs/api-reference/io/index.rst Outdated
maxnoe
maxnoe previously approved these changes Sep 22, 2025
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.

Minor comment on the docs text. Code-wise this looks fine for me now, but I'd like to ping @FrancaCassol for a review here as the original author of most of the calibration code.

Comment thread src/ctapipe/io/hdf5monitoringsource.py
@FrancaCassol
Copy link
Copy Markdown
Contributor

FrancaCassol commented Sep 25, 2025

Hi, the approach of this PR is fine in general. I have just a question, some of the added classes and tools seems to me more specific to calibPipe than to ctapipe, they will be moved to calibPipe later or the idea it to keep most of the code in one library?

@TjarkMiener
Copy link
Copy Markdown
Member Author

TjarkMiener commented Sep 25, 2025

Hi, the approach of this PR is fine in general. I have just a question, some of the added classes and tools seems to me more specific to calibPipe than to ctapipe, they will be moved to calibPipe later or the idea it to keep most of the code in one library?

Hi @FrancaCassol! Thanks for having a look at the PR. The classes and tools that are currently in ctapipe are those that either already serve, or could serve in the future, more general purposes, such as aggregating statistics, detecting outliers, pixel monitoring, chunk interpolation, and so on. calibPipe, on the other hand, is intended to hold only the tools specifically related to calculating the camera calibration coefficients for this use case. Additionally, the interface via MonitoringSource fits naturally in the middleware layer.

@TjarkMiener TjarkMiener requested a review from maxnoe September 25, 2025 09:54
@FrancaCassol
Copy link
Copy Markdown
Contributor

Hi @FrancaCassol! Thanks for having a look at the PR. The classes and tools that are currently in ctapipe are those that either already serve, or could serve in the future, more general purposes, such as aggregating statistics, detecting outliers, pixel monitoring, chunk interpolation, and so on. calibPipe, on the other hand, is intended to hold only the tools specifically related to calculating the camera calibration coefficients for this use case. Additionally, the interface via MonitoringSource fits naturally in the middleware layer.

I see, some of the classes seems to me pretty specific to monitoring issues, but indeed it is never easy to decide where to put interfaces. My only worry is that this development is mainly based on the LST calibration camera concept. For other cameras, things will most probably work differently, so I would have kept the interface at a higher level in order to guarantee independency to further calibPipe developments. That said, I am going to approve this ;-)

FrancaCassol
FrancaCassol previously approved these changes Sep 25, 2025
mexanick
mexanick previously approved these changes Sep 25, 2025
Copy link
Copy Markdown
Contributor

@mexanick mexanick left a comment

Choose a reason for hiding this comment

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

LGTM

maxnoe
maxnoe previously approved these changes Sep 25, 2025
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.

@kosack ok for me too now, please have a final look and merge if ok

@TjarkMiener
Copy link
Copy Markdown
Member Author

@kosack ok for me too now, please have a final look and merge if ok

Hi @kosack, when you have a moment this week, could you please take a look at this PR? Your feedback would be really helpful. Let me know if you need anything from my side. It would be good if we can proceed this week with the integrating of the MonitoringSource within data/calibpipe! Many thanks in advance!

Copy link
Copy Markdown
Member

@kosack kosack left a comment

Choose a reason for hiding this comment

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

Looks very good, other than a few issues and one bug that I found.

Comment thread pyproject.toml Outdated
Comment thread src/ctapipe/instrument/subarray.py
Comment thread src/ctapipe/tools/process.py
Co-authored-by: Karl Kosack <kosack@users.noreply.github.com>
@TjarkMiener TjarkMiener dismissed stale reviews from maxnoe, mexanick, and FrancaCassol via 469c041 October 7, 2025 11:11
Comment thread src/ctapipe/tools/tests/test_process.py
TjarkMiener and others added 2 commits October 7, 2025 13:27
Co-authored-by: Karl Kosack <kosack@users.noreply.github.com>
@ctao-sonarqube
Copy link
Copy Markdown

ctao-sonarqube Bot commented Oct 7, 2025

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