Upgrade nectarchain to ctapipe 0.19 #68

jlenain · 2023-06-16T14:11:30Z

This PR, work in progress, is meant to upgrade nectarchain to ctapipe v0.19, for all code components.

codecov · 2023-06-16T14:23:08Z

Codecov Report

Patch coverage has no change and project coverage change: -0.01 ⚠️

Comparison is base (c67c57f) 0.38% compared to head (f9314c1) 0.38%.

Additional details and impacted files

@@            Coverage Diff            @@
##           master     #68      +/-   ##
=========================================
- Coverage    0.38%   0.38%   -0.01%     
=========================================
  Files          30      30              
  Lines        3350    3404      +54     
=========================================
  Hits           13      13              
- Misses       3337    3391      +54

Impacted Files	Coverage Δ
...ain/calibration/NectarGain/SPEfit/NectarGainSPE.py	`0.00% <0.00%> (ø)`
...ation/NectarGain/SPEfit/NectarGainSPE_singlerun.py	`0.00% <0.00%> (ø)`
src/nectarchain/calibration/container/charge.py	`0.00% <0.00%> (ø)`
src/nectarchain/calibration/container/utils.py	`0.00% <0.00%> (ø)`
src/nectarchain/calibration/container/waveforms.py	`0.00% <0.00%> (ø)`
src/nectarchain/dqm/charge_integration.py	`0.00% <0.00%> (ø)`
src/nectarchain/dqm/mean_camera_display.py	`0.00% <0.00%> (ø)`
src/nectarchain/dqm/start_calib.py	`0.00% <0.00%> (ø)`

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

… the conda-forge package to be built.

…tead of a pip dependency (was a temporary hack, until the last release of ctapipe_io_nectarcam was published as a conda-forge package).

…events in waveformsContainer(s) - Update to ctapipe0.19

…ately

(Next commit should ake care of flagged bad pixels and how to deal with them)

(Fixing bad pixel handling hsould come next)

guillaumegrolleron · 2023-06-19T23:15:51Z

I detected some issues with the astropy upgrade (the version needed by ctapipe has changed). Please do not merge until I solved this problem !

jlenain · 2023-06-20T06:54:25Z

I detected some issues with the astropy upgrade (the version needed by ctapipe has changed). Please do not merge until I solved this problem !

Hi @guillaumegrolleron , no problem. What kind of issues did you spot? Indeed, we are moving from astropy v4 to v5.

review-notebook-app · 2023-06-20T10:48:28Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

jlenain · 2023-06-29T21:16:53Z

Hi @hashkar ,
I am testing several aspects for what concerns this upgrade.
I don't know whether this is related to the upgrade to ctapipe v0.19.1, but for the DQM, on recent runs (e.g. 4415, 4414 or 4410), I got the following error:

Traceback (most recent call last):
  File "/opt/cta/nectarchain/src/nectarchain/dqm/start_calib.py", line 181, in <module>
    Chan, Samp = p.DefineForRun(reader1)
                 ^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/cta/nectarchain/src/nectarchain/dqm/dqm_summary_processor.py", line 41, in DefineForRun
    self.Samp = len(evt1.r0.tel[0].waveform[0][0])
                    ~~~~~~~~~~~~~~~~~~~~~~~^^^
TypeError: 'NoneType' object is not subscriptable

right when grabbing the run configuration through DefineForRun. Do you have an idea about what is going on ?

sizun · 2023-06-30T07:22:40Z

The fact that these runs were taken with gain selection might be a factor.

tibaldo · 2023-06-30T08:16:01Z

Indeed, the r0 container is not filled for gain-selected runs, that's why r0 is a 'NoneType' object. To read gain-selected files you should disable the r0->r1 corrections and then read the raw data in the r1 container.

jlenain · 2023-06-30T08:22:07Z

OK, thanks @sizun , @tibaldo !
@hashkar , this linked to #64 , could you please implement cta-observatory/ctapipe_io_nectarcam#34 (comment) in the DQM, then ?
Many thanks!

guillaumegrolleron · 2023-07-05T11:32:53Z

I detected some issues with the astropy upgrade (the version needed by ctapipe has changed). Please do not merge until I solved this problem !

Finally I solved this problem very easily. I have computed again the charge and it appears that the parameters estimators for the fit initialization didn't work. I fixed this problem and the SPE fit works well now.

jlenain · 2023-07-05T16:20:18Z

I detected some issues with the astropy upgrade (the version needed by ctapipe has changed). Please do not merge until I solved this problem !

Finally I solved this problem very easily. I have computed again the charge and it appears that the parameters estimators for the fit initialization didn't work. I fixed this problem and the SPE fit works well now.

Hi @guillaumegrolleron ,
Thanks a lot!
Do you need to push a commit, or was it just a matter of re-generating the pre-processed charge inputs?

guillaumegrolleron · 2023-07-05T17:55:27Z

Hi @jlenain,
I have to push some commits, I will do on tomorrow

…il in testing the code for different python versions.

tibaldo

There are several entirely new files and many modifications for which to me it's hard to see the relationship with the upgrade to ctapipe 0.19. It's hard to review the code for me since I miss the context. If other changes/enhancements were added along with the modifications for the upgrade of ctapipe it would be useful to provide the relevant information in the PR for future documentation.

src/nectarchain/calibration/NectarGain/SPEfit/NectarGainSPE.py

src/nectarchain/user_scripts/vmarandon/CalibrationData.py

src/nectarchain/user_scripts/vmarandon/ShowDataContent.py

src/nectarchain/user_scripts/vmarandon/ShowPedestalEvolution.py

src/nectarchain/user_scripts/vmarandon/Utils.py

jlenain · 2023-07-11T14:05:15Z

There are several entirely new files and many modifications for which to me it's hard to see the relationship with the upgrade to ctapipe 0.19. It's hard to review the code for me since I miss the context. If other changes/enhancements were added along with the modifications for the upgrade of ctapipe it would be useful to provide the relevant information in the PR for future documentation.

Hum, it seems the last commits by @guillaumegrolleron merged modifications coming from other PR (such as e.g. #70 ). @guillaumegrolleron , would it be possible to undo/clean the commit history of your last contribution, please ?

-update SPE fit parameters initialization following updates of the ctapipe extractors -improve the limits of generated charge histogram plots

guillaumegrolleron · 2023-07-12T09:06:47Z

Hi all,
The commit history has been cleaned, but @jlenain please let me know if there is still any trouble.

jlenain · 2023-07-12T09:39:27Z

Hi all, The commit history has been cleaned, but @jlenain please let me know if there is still any trouble.

Hi @guillaumegrolleron , thanks a lot for that! This is all fine now.

src/nectarchain/calibration/NectarGain/SPEfit/NectarGainSPE_singlerun.py

tibaldo

There are still a few changes in the DQM part for which I'm not sure I understand the relationship with the upgrade to ctapipe 0.19

src/nectarchain/dqm/charge_integration.py

src/nectarchain/dqm/mean_camera_display.py

jlenain · 2023-07-12T14:25:23Z

Thanks a lot for the review, @tibaldo !

jlenain mentioned this pull request Jun 16, 2023

Ctapipe upgrade to 0.19 #67

Closed

jlenain and others added 10 commits June 17, 2023 23:17

Adapt package dependencies to prepare for ctapipe-0.19

4ac4475

Upgrade to ctapipe 0.19.2

164713c

Pin ctapipe_io_nectarcam version to 0.1.3

fddc25b

Install pinned version of ctapipe-io-nectarcam via PyPI, the time for…

93c3ca6

… the conda-forge package to be built.

Fixed typo

989f209

Put ctapipe_io_nectarcam dependency back into conda dependencies, ins…

ae0faa8

…tead of a pip dependency (was a temporary hack, until the last release of ctapipe_io_nectarcam was published as a conda-forge package).

Pin ctapipe 0.19.1 (same as for ctapipe_io_nectarcam)

42b1f9d

- minimize the RAM needed by not initializing arrays before reading …

f112f02

…events in waveformsContainer(s) - Update to ctapipe0.19

Bump ctapipe version into CI tests.

c38d933

COMDIRAC is already embedded in DIRAC v8, no need to install it separ…

5b82634

…ately

jlenain force-pushed the ctapipe0.19 branch from 0933d35 to 5b82634 Compare June 17, 2023 21:17

hashkar added 2 commits June 19, 2023 11:04

upgrading DQM to CTAPIPE 0.19.

8f882ea

(Next commit should ake care of flagged bad pixels and how to deal with them)

upgrading DQM to CTAPIPE 0.19

d90760d

(Fixing bad pixel handling hsould come next)

jlenain added the enhancement New feature or request label Jun 20, 2023

jlenain marked this pull request as draft June 20, 2023 10:06

Updated notebook

ae1a052

jlenain linked an issue Jun 20, 2023 that may be closed by this pull request

Move to latest ctapipe for dependency (ctapipe-0.19) #46

Closed

bug fic for mean_camera_display.py

51fc818

jlenain mentioned this pull request Jun 28, 2023

Show how to pin container version in jobs #73

Merged

Jean-Philippe Lenain added 2 commits July 6, 2023 12:15

Change the way the python version is specified, otherwise CI tests fa…

8f16170

…il in testing the code for different python versions.

Python 3.10 interpreted as 3.1 in CI

9b4fc9e

jlenain marked this pull request as ready for review July 11, 2023 10:46

tibaldo reviewed Jul 11, 2023

View reviewed changes

clean commit history :

5c05525

-update SPE fit parameters initialization following updates of the ctapipe extractors -improve the limits of generated charge histogram plots

guillaumegrolleron force-pushed the ctapipe0.19 branch from dcae747 to 5c05525 Compare July 12, 2023 09:03

tibaldo reviewed Jul 12, 2023

View reviewed changes

src/nectarchain/calibration/NectarGain/SPEfit/NectarGainSPE_singlerun.py Outdated Show resolved Hide resolved

remove useless imports

a75d4c9

tibaldo reviewed Jul 12, 2023

View reviewed changes

src/nectarchain/dqm/charge_integration.py Outdated Show resolved Hide resolved

src/nectarchain/dqm/mean_camera_display.py Outdated Show resolved Hide resolved

Revert changes in DQM which actually belong to cta-observatory#63

f9314c1

tibaldo approved these changes Jul 12, 2023

View reviewed changes

jlenain merged commit cdded69 into cta-observatory:master Jul 12, 2023
5 of 8 checks passed

jlenain deleted the ctapipe0.19 branch July 12, 2023 14:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade nectarchain to ctapipe 0.19 #68

Upgrade nectarchain to ctapipe 0.19 #68

jlenain commented Jun 16, 2023

codecov bot commented Jun 16, 2023 •

edited

Loading

guillaumegrolleron commented Jun 19, 2023

jlenain commented Jun 20, 2023 •

edited

Loading

review-notebook-app bot commented Jun 20, 2023

jlenain commented Jun 29, 2023 •

edited

Loading

sizun commented Jun 30, 2023

tibaldo commented Jun 30, 2023

jlenain commented Jun 30, 2023 •

edited

Loading

guillaumegrolleron commented Jul 5, 2023 •

edited

Loading

jlenain commented Jul 5, 2023

guillaumegrolleron commented Jul 5, 2023

tibaldo left a comment

jlenain commented Jul 11, 2023

guillaumegrolleron commented Jul 12, 2023

jlenain commented Jul 12, 2023

tibaldo left a comment

jlenain commented Jul 12, 2023

Upgrade nectarchain to ctapipe 0.19 #68

Upgrade nectarchain to ctapipe 0.19 #68

Conversation

jlenain commented Jun 16, 2023

codecov bot commented Jun 16, 2023 • edited Loading

Codecov Report

guillaumegrolleron commented Jun 19, 2023

jlenain commented Jun 20, 2023 • edited Loading

review-notebook-app bot commented Jun 20, 2023

jlenain commented Jun 29, 2023 • edited Loading

sizun commented Jun 30, 2023

tibaldo commented Jun 30, 2023

jlenain commented Jun 30, 2023 • edited Loading

guillaumegrolleron commented Jul 5, 2023 • edited Loading

jlenain commented Jul 5, 2023

guillaumegrolleron commented Jul 5, 2023

tibaldo left a comment

Choose a reason for hiding this comment

jlenain commented Jul 11, 2023

guillaumegrolleron commented Jul 12, 2023

jlenain commented Jul 12, 2023

tibaldo left a comment

Choose a reason for hiding this comment

jlenain commented Jul 12, 2023

codecov bot commented Jun 16, 2023 •

edited

Loading

jlenain commented Jun 20, 2023 •

edited

Loading

jlenain commented Jun 29, 2023 •

edited

Loading

jlenain commented Jun 30, 2023 •

edited

Loading

guillaumegrolleron commented Jul 5, 2023 •

edited

Loading