Skip to content

Conversation

@jlenain
Copy link
Collaborator

@jlenain jlenain commented May 21, 2025

This PR implements the needed change to upgrade nectarchain to depend on ctapipe version 0.24 instead of 0.19.

Most notably, this mostly boils down to ImageExtractors in ctapipe which can now support waveforms of shapes (n_gain, n_pixel, n_sample) additionally to (n_pixel, n_sample).

@guillaumegrolleron
Copy link
Contributor

guillaumegrolleron commented May 21, 2025

Hi @jlenain,
I think to have fixed the problem in the unit tests. The point was related to the PR cta-observatory/ctapipe#2529, where ctapipe extractors can now deal with waveforms with shape (n_channel, n_pixels, n_samples). The output in DL1CameraContainer is then quite different.
The modifications in nectarchain are small because of the transparency use of extractors

jlenain added 2 commits May 22, 2025 09:25
@jlenain
Copy link
Collaborator Author

jlenain commented May 22, 2025

Hi @jlenain, I think to have fixed the problem in the unit tests. The point was related to the PR cta-observatory/ctapipe#2529, where ctapipe extractors can now deal with waveforms with shape (n_channel, n_pixels, n_samples). The output in DL1CameraContainer is then quite different. The modifications in nectarchain are small because of the transparency use of extractors

Hi @guillaumegrolleron ,
Many thanks for that !
I also had to ensure the member pixel_mask in PedestalEstimationComponent was of the expected type to fix the remaining unit tests.
I am still not sure whether we need to enforce the shape of waveforms to (n_gain, n_pixel, n_sample) in ctapipe_io_nectarcam for all cases (see https://github.com/cta-observatory/ctapipe_io_nectarcam/pull/70/files#diff-c1dfa027cc2f2c588325ed4878a39479d6483636e83523c960496fbf1cbc62adL1190 and https://github.com/cta-observatory/ctapipe_io_nectarcam/pull/70/files#diff-c1dfa027cc2f2c588325ed4878a39479d6483636e83523c960496fbf1cbc62adL1197) or not. @vmarandon , would you have an idea ?

Now, the CI workflow still fails on macOS, but this is related to another issue to be fixed (proper installation of protozfits).

@codecov
Copy link

codecov bot commented May 22, 2025

Codecov Report

Attention: Patch coverage is 48.48485% with 17 lines in your changes missing coverage. Please review.

Project coverage is 52.25%. Comparing base (c4dfdec) to head (4a40634).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/nectarchain/dqm/charge_integration.py 0.00% 11 Missing ⚠️
src/nectarchain/trr_test_suite/gui.py 25.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #191      +/-   ##
==========================================
- Coverage   52.49%   52.25%   -0.25%     
==========================================
  Files          76       76              
  Lines        6311     6329      +18     
==========================================
- Hits         3313     3307       -6     
- Misses       2998     3022      +24     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jlenain
Copy link
Collaborator Author

jlenain commented Jun 18, 2025

I checked this PR still work with:

  • the TRR test suite
  • the DQM, after a minor fix (see b942821, @hashkar could you please check ?)

I think this PR is ready for review.

Beware, it should not be merged before fixing:

@guillaumegrolleron , @vmarandon , @frnbrun , anyone willing to review ?

@jlenain jlenain marked this pull request as ready for review June 18, 2025 15:55
@jlenain jlenain linked an issue Jun 18, 2025 that may be closed by this pull request
@guillaumegrolleron
Copy link
Contributor

I checked this PR still work with:

* the TRR test suite

* the DQM, after a minor fix (see [b942821](https://github.com/cta-observatory/nectarchain/commit/b94282132f8fd5188d8ed805446ae201f94d012d), @hashkar could you please check ?)

I think this PR is ready for review.

Beware, it should not be merged before fixing:

* https://github.com/cta-observatory/nectarchain/pull/191/files#diff-9efd195f4e9bfb79ccd456a1d8370fafcc4bcb0b00ea3799222667d2ae818533R8

* https://github.com/cta-observatory/nectarchain/pull/191/files#diff-9efd195f4e9bfb79ccd456a1d8370fafcc4bcb0b00ea3799222667d2ae818533R25

@guillaumegrolleron , @vmarandon , @frnbrun , anyone willing to review ?

Hi @jlenain, thanks a lot for your work, I can have a look

jlenain added a commit that referenced this pull request Jun 24, 2025
CI tests fail due to another issue: the dependency to `ctapipe_io_nectarcam` should be pinned to version `0.1`, to avoid installing `ctapipe` v0.24 in CI before #191 is merged.
@guillaumegrolleron guillaumegrolleron self-requested a review June 24, 2025 15:29
Copy link
Contributor

@guillaumegrolleron guillaumegrolleron left a comment

Choose a reason for hiding this comment

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

Hi @jlenain, thanks a lot for your update, I approve the PR

@jlenain
Copy link
Collaborator Author

jlenain commented Jun 26, 2025

Hi @jlenain, thanks a lot for your update, I approve the PR

Many thanks, @guillaumegrolleron !

@jlenain jlenain merged commit 3fe9480 into cta-observatory:main Jun 26, 2025
9 of 11 checks passed
@jlenain jlenain deleted the ctapipe-0.24-bis branch June 26, 2025 12:10
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.

Update to ctapipe 0.20

2 participants