Skip to content

Conversation

@guillaumegrolleron
Copy link
Contributor

@guillaumegrolleron guillaumegrolleron commented Sep 18, 2025

Small improvement of the gain calibration pipeline regarding the last results presented at ICRC2025
The SPE fit parameters in yaml file are currently the most valid.
It most concerns photo-statistic code, where errors on gain are now estimated, and the possibility to use SPE resolution computed from a different SPE fit approach is now properly managed.

Bugfix: ChargesContainers.from_hdf5 -> when loading sliced, only the last one was returned

Bugfix: charge extraction through ctapipe extractor -> wrong output shape (see #215)

This should fix issues raised by @tibaldo, and @hashkar

@jlenain jlenain added bug Something isn't working enhancement New feature or request labels Sep 18, 2025
@jlenain jlenain linked an issue Sep 18, 2025 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Sep 19, 2025

Codecov Report

❌ Patch coverage is 90.00000% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.78%. Comparing base (aee99c6) to head (103c1d2).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
.../nectarchain/makers/calibration/pedestal_makers.py 75.00% 3 Missing ⚠️
...ain/makers/calibration/tests/test_pedestal_tool.py 89.47% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #214      +/-   ##
==========================================
+ Coverage   53.36%   53.78%   +0.41%     
==========================================
  Files          76       76              
  Lines        6234     6264      +30     
==========================================
+ Hits         3327     3369      +42     
+ Misses       2907     2895      -12     

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

Copy link
Collaborator

@jlenain jlenain left a comment

Choose a reason for hiding this comment

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

Thanks a lot, @guillaumegrolleron ! It looks all good to me. I just note that the dimensions of the ChargesContainer (charges and peaks) changed from 4 to 3. I guess it can create misbehaving side effects. Are we good about that ?

@guillaumegrolleron
Copy link
Contributor Author

ChargesContainer

Hi @jlenain,
I would expect the dimension of charge_hg /_lg to be 2, where did you get a 3 ?

@jlenain
Copy link
Collaborator

jlenain commented Sep 22, 2025

Hum, just reading the modified code, at https://github.com/cta-observatory/nectarchain/pull/214/files#diff-312de2737781494f8f84079d289befcf6226569609a674d088050dee94082f52R604. We have out[0, ...] and out[1, ...], while before we had e.g. out[:, 0, channel, :], but maybe the ellipsis fooled me.

@jlenain
Copy link
Collaborator

jlenain commented Sep 22, 2025

Testing the script gain_PhotoStat_computation.py, I got an argument error:

$ python gain_PhotoStat_computation.py --FF_run_number 6807 --Ped_run_number 6806 --SPE_run_number 6852 --SPE_config HHVfixed --method LocalPeakWindowSum --extractor_kwargs '{"window_width":8,"window_shift":4}' --overwrite -v INFO
/home/jlenain/local/opt/conda/envs/nectar-dev/lib/python3.11/site-packages/DIRAC/__init__.py:63: UserWarning: pkg_resources is deprecated as an API. See https://setuptools.pypa.io/en/latest/pkg_resources.html. The pkg_resources package is slated for removal as early as 2025-11-30. Refrain from using this package or pin to Setuptools<81.
  from pkg_resources import get_distribution, DistributionNotFound
/home/jlenain/local/opt/conda/envs/nectar-dev/lib/python3.11/site-packages/ctapipe/instrument/camera/geometry.py:616: FromNameWarning: .from_name uses pre-defined data that is likely different from the data being analyzed. Access instrument information via the SubarrayDescription instead.
  warn_from_name()
Traceback (most recent call last):
  File "/home/jlenain/local/src/python/cta-observatory/nectarchain/src/nectarchain/user_scripts/ggrolleron/gain_PhotoStat_computation.py", line 103, in <module>
    parser.add_argument(
  File "/home/jlenain/local/opt/conda/envs/nectar-dev/lib/python3.11/argparse.py", line 1455, in add_argument
    action = action_class(**kwargs)
             ^^^^^^^^^^^^^^^^^^^^^^
TypeError: _StoreAction.__init__() got an unexpected keyword argument 'description'

parser.add_argument should be passed a help option, instead of description, I guess.

@guillaumegrolleron
Copy link
Contributor Author

guillaumegrolleron commented Sep 22, 2025

out[:, 0, channel, :]

Before

CtapipeExtractor.get_image_peak_time(
                   imageExtractor(
                       waveforms=waveforms,
                       tel_id=tel_id,
                       selected_gain_channel=None,
                       broken_pixels=broken_pixels,
                   )

return an array of shape (n_channel, n_events, n_pixels). Then, because of the transformation into a list before an array, the dimension was artificially increased to 4, with a shape (1,n_channel, n_events, n_pixels). Then the acces to out[:, 0, channel, :] return an array of shape (n_events, n_pixels). Now that the passage through a list has been removed, then the access with [0,...] is fine, if I'm correct.

@guillaumegrolleron
Copy link
Contributor Author

Testing the script gain_PhotoStat_computation.py, I got an argument error:

$ python gain_PhotoStat_computation.py --FF_run_number 6807 --Ped_run_number 6806 --SPE_run_number 6852 --SPE_config HHVfixed --method LocalPeakWindowSum --extractor_kwargs '{"window_width":8,"window_shift":4}' --overwrite -v INFO
/home/jlenain/local/opt/conda/envs/nectar-dev/lib/python3.11/site-packages/DIRAC/__init__.py:63: UserWarning: pkg_resources is deprecated as an API. See https://setuptools.pypa.io/en/latest/pkg_resources.html. The pkg_resources package is slated for removal as early as 2025-11-30. Refrain from using this package or pin to Setuptools<81.
  from pkg_resources import get_distribution, DistributionNotFound
/home/jlenain/local/opt/conda/envs/nectar-dev/lib/python3.11/site-packages/ctapipe/instrument/camera/geometry.py:616: FromNameWarning: .from_name uses pre-defined data that is likely different from the data being analyzed. Access instrument information via the SubarrayDescription instead.
  warn_from_name()
Traceback (most recent call last):
  File "/home/jlenain/local/src/python/cta-observatory/nectarchain/src/nectarchain/user_scripts/ggrolleron/gain_PhotoStat_computation.py", line 103, in <module>
    parser.add_argument(
  File "/home/jlenain/local/opt/conda/envs/nectar-dev/lib/python3.11/argparse.py", line 1455, in add_argument
    action = action_class(**kwargs)
             ^^^^^^^^^^^^^^^^^^^^^^
TypeError: _StoreAction.__init__() got an unexpected keyword argument 'description'

parser.add_argument should be passed a help option, instead of description, I guess.

Thanks! It is fixed

@jlenain
Copy link
Collaborator

jlenain commented Sep 22, 2025

out[:, 0, channel, :]

Before

CtapipeExtractor.get_image_peak_time(
                   imageExtractor(
                       waveforms=waveforms,
                       tel_id=tel_id,
                       selected_gain_channel=None,
                       broken_pixels=broken_pixels,
                   )

return an array of shape (n_channel, n_events, n_pixels). Then, because of the transformation into a list before an array, the dimension was artificially increased to 4, with a shape (1,n_channel, n_events, n_pixels). Then the acces to out[:, 0, channel, :] return an array of shape (n_events, n_pixels). Now that the passage through a list has been removed, then the access with [0,...] is fine, if I'm correct.

Argh, I should think a bit more before writing comments on GitHub, it all makes sense, thanks a lot !

@jlenain
Copy link
Collaborator

jlenain commented Sep 26, 2025

Hi @guillaumegrolleron !

I tested again several scipts using this PR and everything seem to run fine, thanks a lot again !
Are we good to merge it, or did you want to push other things ?

@guillaumegrolleron
Copy link
Contributor Author

Hi @guillaumegrolleron !

I tested again several scipts using this PR and everything seem to run fine, thanks a lot again ! Are we good to merge it, or did you want to push other things ?

Hi @jlenain, Thank you for the review!
It's ok, I have nothing to add.

@guillaumegrolleron guillaumegrolleron merged commit 1e95f07 into cta-observatory:main Oct 1, 2025
11 checks passed
tibaldo pushed a commit to tibaldo/nectarchain that referenced this pull request Oct 2, 2025
…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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ChargesComponent.create_from_waveforms output has wrong dimensions

2 participants