Skip to content

Fix bug in ChargeDistributionFilter for pedestal maker #213

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 10 commits into from
Oct 3, 2025
Merged

Fix bug in ChargeDistributionFilter for pedestal maker #213

merged 10 commits into from
Oct 3, 2025

Conversation

@tibaldo
Copy link
Member

@tibaldo tibaldo commented Sep 16, 2025

While analyzing some data I had a crash. I found out this was due to the fact that a piece of code was misplaced. I moved the piece of code to create a charge container after the check that the waveforms are available. This avoids a crash in some special cases like the one I had encountered.

@codecov
Copy link

codecov bot commented Sep 16, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 77.77778% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 51.79%. Comparing base (1e95f07) to head (0b2df68).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...nectarchain/makers/component/pedestal_component.py 75.00% 2 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #213   +/-   ##
=======================================
  Coverage   51.79%   51.79%           
=======================================
  Files          78       78           
  Lines        6505     6505           
=======================================
  Hits         3369     3369           
  Misses       3136     3136

☔ 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.

@tibaldo tibaldo marked this pull request as draft September 16, 2025 16:34
@jlenain
Copy link
Collaborator

jlenain commented Sep 17, 2025 •
edited
Loading

Hi @tibaldo ,

Thanks a lot for that, it all makes sense !
Can you please elaborate in which case you encountered such an issue (maybe just a run number, or command line that you used) ? We indeed have some NectarCAM runs, that are not documented in the Elog, which are emtpy (no Events field in the .fits.gz files). Did it occur with one of those, or in another situation ?

@jlenain
Copy link
Collaborator

jlenain commented Sep 22, 2025

Hi @tibaldo !
If this PR is ready, could you please undraft it ? Is it ready to be merged on your side ?

@jlenain jlenain self-requested a review September 26, 2025 12:46
@jlenain jlenain added the bug Something isn't working label Sep 26, 2025
@tibaldo
Copy link
Member Author

tibaldo commented Sep 26, 2025

Hi @jlenain, the crash happened for run 7020. I think it was due to an ill choice of mine for the parameters max_events and events_per_slice which ended up creating an empy slice.
As for undrafting: I was waiting on #214 to be merged so that I can more thouroughly test everything is okay.

@jlenain
Copy link
Collaborator

jlenain commented Oct 2, 2025

Hi @jlenain, the crash happened for run 7020. I think it was due to an ill choice of mine for the parameters max_events and events_per_slice which ended up creating an empy slice. As for undrafting: I was waiting on #214 to be merged so that I can more thouroughly test everything is okay.

Hi @tibaldo ,
#214 has now been merged (thanks, @guillaumegrolleron !), so you can go ahead whenever you want/can.

Thanks !

jlenain and others added 4 commits October 2, 2025 12:16
@jlenain @tibaldo
Launch TRR test suite GUI not in full screen (cta-observatory#216)
250ee8c
@pcorcam @tibaldo
Addition of CalibrationWriterNectarCAM tool for writing Category A ca…
c4892ed 
…libration files (cta-observatory#207)

* Add script to obtain metadata of `ctapipe` and `nectarchain`. Taken from `lstcam_calib` with minor adaptations for `nectarchain`.

* added group name as option to HDF5 readout, particularly useful for pedestal output files that use various groups to store data

* first attempt for a Tool that writes a category A output file from NectarCAM containers

* Add example script for the usage of the CalibrationWriterNectarCAM tool

* ensure that filled arrays have the correct type (based on `lstcam_calib` example file)

* do one-padding instead of zero-padding to expand arrays related to pixel status with missing pixels (so a missing pixel = True for those that are added, i.e. due that were not included due to hardware failure)

* fixed hardware failing pixel status bug
@guillaumegrolleron @tibaldo
Improvement of the gain calibration + Bugfix of charge extraction (ct…
f5c6fda 
…a-observatory#214)

* change defaut value of n and pp from HHV the free fit

* user script to produce ICRC2025 results

* Bug fix FF SPE combined

* add gain err to photostat method -> propagation of SPE resolution error

* follows photostat error

* logging improbemeltb

* fix name bug in charge makers

* -change pp and n to be taken from config file instead of SPE HHV result in SPE combined algo
-better management of SPE res used for PS method

* bugfix : wrong position of the yeild within a loop -> generator returned was pointing the last slice

* typo

* bugfix : when extracting charge and peak time with ctapipe extractor, even if selected_gain_channel is none, the expected sample axis is wafevorms.shape[-1]. Then if we provide (n_events,n_pix,n_sample) the output is (n_events, n_pix)

* imrove unit test to detect the issue related to charge extraction

* imporove debugging of scripts

* formatting

* fix pedestal makers and component according to last update of the containers I/O methods + unit test fix

* linter

* fix unit test of charge and waveforms makers
improve behavior if event_per_slice > n_events iin data

* fix gain pstat strict arguments

---------

Co-authored-by: Guillaume Grolleron <[email protected]>
@tibaldo
Merge branch 'cta-observatory:main' into debug_pedestal_chargefilter
46245bd
@tibaldo
Copy link
Member Author

tibaldo commented Oct 2, 2025

Hi @jlenain : I merged the latest main branch and tested on my Mac, which worked fine. There are some CI errors though, do you have any ideas about the cause and how to solve them?

@jlenain
Copy link
Collaborator

jlenain commented Oct 2, 2025 •
edited
Loading

Hi @jlenain : I merged the latest main branch and tested on my Mac, which worked fine. There are some CI errors though, do you have any ideas about the cause and how to solve them?

It is a circular import issue due to the import of ZODB for macOS runners... (https://github.com/cta-observatory/nectarchain/actions/runs/18190146025/job/51788261186?pr=213#step:12:45)

Could you please let me know which version you currently have for ZODB in your working conda environment ? We may have to pin a version there for macOS. Currently, we don't pin it.

@tibaldo
Copy link
Member Author

tibaldo commented Oct 2, 2025

I have ZODB 6.0.1 on MacOS Sequoia 15.6.1.
Are you suggesting that I pin it in the environment file within this pull request?

@jlenain
Copy link
Collaborator

jlenain commented Oct 2, 2025

I have ZODB 6.0.1 on MacOS Sequoia 15.6.1. Are you suggesting that I pin it in the environment file within this pull request?

Thanks !
Let me try to do it within your PR.
I see they released a new version (v6.1) yesterday (https://pypi.org/project/ZODB/). That might be the issue.

@jlenain
Copy link
Collaborator

jlenain commented Oct 2, 2025

Hi @tibaldo ,
OK, this is fixed (needed to pin the version of a sub-dependency package for ZODB)

@tibaldo tibaldo marked this pull request as ready for review October 3, 2025 07:18
@tibaldo
Copy link
Member Author

tibaldo commented Oct 3, 2025

Thanks a bunch @jlenain, the PR is ready for merging I think

@jlenain
Copy link
Collaborator

jlenain commented Oct 3, 2025

Thanks a lot, @tibaldo !

@jlenain jlenain merged commit 09b87e2 into cta-observatory:main Oct 3, 2025
11 checks passed
@jlenain jlenain deleted the debug_pedestal_chargefilter branch October 3, 2025 07:38
tibaldo added a commit to tibaldo/nectarchain that referenced this pull request Oct 8, 2025
@tibaldo @jlenain
Fix bug in ChargeDistributionFilter for pedestal maker (cta-observato…
c36c2a6 
…ry#213)

* Only fill charges container if waveforms are available

* Cleanup commented stuff from some previous PR

* Pin `persistent` (dependency for `ZODB`) version to fix CI

---------

Co-authored-by: Jean-Philippe Lenain <[email protected]>
# Conflicts:
#	src/nectarchain/makers/component/pedestal_component.py
tibaldo added a commit to tibaldo/nectarchain that referenced this pull request Oct 8, 2025
@tibaldo @jlenain
Fix bug in ChargeDistributionFilter for pedestal maker (cta-observato…
79033a7 
…ry#213)

* Only fill charges container if waveforms are available

* Cleanup commented stuff from some previous PR

* Pin `persistent` (dependency for `ZODB`) version to fix CI

---------

Co-authored-by: Jean-Philippe Lenain <[email protected]>
# Conflicts:
#	src/nectarchain/makers/component/pedestal_component.py
tibaldo added a commit to tibaldo/nectarchain that referenced this pull request Oct 8, 2025
@tibaldo @jlenain
Fix bug in ChargeDistributionFilter for pedestal maker (cta-observato…
2c24faf 
…ry#213)

* Only fill charges container if waveforms are available

* Cleanup commented stuff from some previous PR

* Pin `persistent` (dependency for `ZODB`) version to fix CI

---------

Co-authored-by: Jean-Philippe Lenain <[email protected]>
# Conflicts:
#	src/nectarchain/makers/component/pedestal_component.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jlenain jlenain jlenain approved these changes

Assignees

@tibaldo tibaldo

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tibaldo @jlenain @pcorcam @guillaumegrolleron