Skip to content
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.

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

New implementation of the nectarchain module #82

Merged
merged 130 commits into from
Jan 22, 2024

Conversation

guillaumegrolleron
Copy link
Contributor

In this PR, I finished to re-implement the nectarchain module following the ctapipe framework.

We are no longer using format defined in nectarchain to save waveforms, charges or calibrations coefficients. Now all the data are based on the 'ctapipe.containers' classes. Everything is stored in hdf5 format.

Moreover, a general skeleton has been developed ('EventsLoopNectarCAMCalibrationTool'), based on the ctapipe.tool feature. Basically this top class has a Component member that can be overload, which contains the names of the ctapipe component that will be apply on each events. The goal is to make future development easier, we only need to write new component which will perform the work. The whole management of the loop over events as well as the writing to output file is managed by the nectarchain module. A Tutorial notebook to explain how to develop new features through the Component workflow will come in upcoming commits.

I have yet completely rewrite the waveforms, charges extraction, and the gain computation (with both SPE fit and photo-statistic method) following this new implementation.

I also wrote a lighter EventSource than the one that we were using from ctapipe_io_nectarcam to speed up the loop over events.

There are some technical points for which I would like to have the point of view of ctapipe experts, such as the transmission of the configurable traits defined in component to the tool containing that component, to make them visible at the tool creation level.

Some points are still missing as will come in an upcoming PR :
-Unit test rewriting
-Documentation
-cleaning + change module names following PEP recommendations
-WT SPE reimplementation

guillaume.grolleron added 30 commits September 13, 2023 20:06
-update SPE fit parameters initialization
following updates of the ctapipe extractors
-improve the limits of generated charge histogram plots
and ChargesContainer following events_id
-container module for the data structure (to be extend)
-makers module to make the computation of calibration quantities
 on the data structure
at nominal voltage
+ update of user script
user script updated : FF SPE at VVH voltage (ie for single run)
update for nominal FF SPE fit
the fitting process continues although the initial
 parameters computation failed
Copy link

codecov bot commented Nov 24, 2023

Codecov Report

Attention: 762 lines in your changes are missing coverage. Please review.

Comparison is base (12a5cc7) 7.35% compared to head (4961c02) 30.56%.
Report is 1 commits behind head on master.

Files Patch % Lines
.../nectarchain/makers/component/spe/spe_algorithm.py 32.58% 120 Missing ⚠️
src/nectarchain/makers/core.py 19.83% 97 Missing ⚠️
...chain/makers/component/photostatistic_algorithm.py 40.00% 87 Missing ⚠️
src/nectarchain/data/container/eventSource.py 17.82% 83 Missing ⚠️
src/nectarchain/data/container/core.py 20.65% 73 Missing ⚠️
...rchain/makers/calibration/gain/photostat_makers.py 35.44% 51 Missing ⚠️
...hain/makers/calibration/gain/FlatFieldSPEMakers.py 46.75% 41 Missing ⚠️
src/nectarchain/data/management.py 14.63% 35 Missing ⚠️
...chain/makers/component/photostatistic_component.py 43.85% 32 Missing ⚠️
src/nectarchain/makers/chargesMakers.py 41.50% 31 Missing ⚠️
... and 9 more
Additional details and impacted files
@@             Coverage Diff             @@
##           master      #82       +/-   ##
===========================================
+ Coverage    7.35%   30.56%   +23.21%     
===========================================
  Files          45       58       +13     
  Lines        2978     3700      +722     
===========================================
+ Hits          219     1131      +912     
+ Misses       2759     2569      -190     

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

guillaume.grolleron added 5 commits November 28, 2023 16:44
-find SPE data
-containers data with maxevents
will return a generator to only
 load the slices one by one during execution
 -> reduce the RAM needed
at nominal voltage (config files)
-change SPE fit plotting matplotlib -> pyqtgraph
 to speed up the plots generation
@jlenain
Copy link
Collaborator

jlenain commented Dec 12, 2023

Hi @guillaumegrolleron ,

Thank you so much for that!

I am currently testing user_scripts/ggrolleron/notebooks/waveforms_charges_extraction_new_framework.py and would have a very small request: could it be possible to create the parent directories $NECTARCAMDATA/runs/charges and $NECTRCAMDATA/runs/waveforms on the fly, if they don't exist, at line 245 in src/nectarchain/makers/core.py? I know this is not related to nectarchain but to ctapipe, but do you know if that may be an option within HDF5TableWriter ?

Otherwise, the tools to extract waveforms and charges work like a charm !

@jlenain
Copy link
Collaborator

jlenain commented Jan 9, 2024

When testing the new implementation, we (@guillaumegrolleron and I) came across one issue: when pre-extracting waveforms and charges from *.fits.fz files and outputting them as HDF5 files using the script load_wfs_compute_charge.py and using these HDF5 files to extract the gain, e.g. with gain_SPEfit_computation.py, it seems the hierarchical structure written by load_wfs_compute_charge.py is not what is expected by ChargesContainers.

For instance:

python load_wfs_compute_charge.py -r 3942 --max_events 100  --method GlobalPeakWindowSum --extractor_kwargs '{"window_width":16,"window_shift":4}' --overwrite -v DEBUG
python gain_SPEfit_computation.py -r 3942 --free_pp_n --multiproc --nproc 8 --chunksize 100  --method GlobalPeakWindowSum --extractor_kwargs '{"window_width":16,"window_shift":4}' -v DEBUG -m 100 --events_per_slice 100

returns:

2024-01-09 10:27:26,884 - __main__ - INFO - arguments passed to main are : {'run_number': [3942], 'max_events': [100], 'reload_events': False, 'overwrite': False, 'events_per_slice': 1000, 'method': 'GlobalPeakWindowSum', 'extractor_kwargs': {'window_width': 16, 'window_shift': 4}, 'asked_pixels_id': None, 'free_pp_n': True, 'multiproc': True, 'nproc': 8, 'chunksize': 1000, 'log_level': 'DEBUG'}
2024-01-09 10:27:26,885 - __main__ - INFO - max_events : [100]
2024-01-09 10:27:26,886 - nectarchain.makers.core - WARNING - the componentName in componentsList must be defined in the nectarchain.makers.component module, otherwise the import of the componentName will raise an error
2024-01-09 10:27:26,899 WARNING [nectarchain.makers.calibration.gain.FlatFieldSPEMakers] (FlatFieldSPEMakers.__init__): You asked events_per_slice but you don't want to reload events and a charges file is on disk, then events_per_slice is set to None
2024-01-09 10:27:26,899 INFO [nectarchain.FlatFieldSPEHHVNectarCAM] (core.setup): setup of the Tool
2024-01-09 10:27:26,900 DEBUG [nectarchain.FlatFieldSPEHHVNectarCAM] (core._load_eventsource): loading event source
2024-01-09 10:27:27,059 INFO [nectarchain.data.management] (management.findrun): Found 4 files matching /data/users/jlenain/tmp/NectarCAM.Run3942.*.fits.fz
2024-01-09 10:27:27,060 INFO [nectarchain.makers.core] (core.load_run): /data/users/jlenain/tmp/NectarCAM.Run3942.*.fits.fz will be loaded
2024-01-09 10:27:27,063 INFO [nectarchain.data.container.eventSource.LightNectarCAMEventSource] (eventsource.__init__): INPUT PATH = /data/users/jlenain/tmp/NectarCAM.Run3942.0000.fits.fz
2024-01-09 10:27:27,063 INFO [nectarchain.data.container.eventSource.LightNectarCAMEventSource] (eventsource.__init__): Max events being read = 100
2024-01-09 10:27:29,675 INFO [nectarchain.data.container.eventSource.LightNectarCAMEventSource] (__init__.__init__): Read 4 input files
2024-01-09 10:27:29,946 INFO [nectarchain.FlatFieldSPEHHVNectarCAM] (core.__setup_components): setup of components
2024-01-09 10:27:29,951 INFO [nectarchain.FlatFieldSPEHHVNectarCAM] (core._init_writer): initilization of writter in full mode
2024-01-09 10:27:29,951 DEBUG [nectarchain.FlatFieldSPEHHVNectarCAM.HDF5TableWriter] (hdf5tableio.open): kwargs for tables.open_file: {'mode': 'w', 'root_uep': '/', 'filters': Filters(complevel=5, complib='blosc:zstd', shuffle=True, bitshuffle=False, fletcher32=True, least_significant_digit=None)}
2024-01-09 10:27:29,955 DEBUG [nectarchain.FlatFieldSPEHHVNectarCAM.HDF5TableWriter] (hdf5tableio.__init__): h5file: /data/users/jlenain/tmp/SPEfit/FlatFieldSPEHHVNectarCAM_run3942_maxevents100_GlobalPeakWindowSum_window_width_16_window_shift_4_sliced1000.h5 (File) ''
Last modif.: '2024-01-09T09:27:29+00:00'
Object Tree: 
/ (RootGroup) ''

2024-01-09 10:27:29,957 INFO [nectarchain.FlatFieldSPEHHVNectarCAM] (FlatFieldSPEMakers.start): reading computed charge from files /data/users/jlenain/tmp/runs/charges/ChargesNectarCAMCalibration_run3942_maxevents100_GlobalPeakWindowSum_window_width_16_window_shift_4.h5
2024-01-09 10:27:29,957 DEBUG [nectarchain.FlatFieldSPEHHVNectarCAM] (FlatFieldSPEMakers.start): merging along slices
2024-01-09 10:27:29,960 INFO [nectarchain.data.container.core] (core._container_from_hdf5): reading ChargesContainers containing a single slice, will return the ChargesContainers instance
2024-01-09 10:27:29,960 WARNING [nectarchain.data.container.core] (core._container_from_hdf5): group ``/`` does not have a child named ``data``
2024-01-09 10:27:29,960 WARNING [nectarchain.data.container.core] (core._container_from_hdf5): group ``/`` does not have a child named ``data``
2024-01-09 10:27:29,961 WARNING [nectarchain.data.container.core] (core._container_from_hdf5): group ``/`` does not have a child named ``data``
2024-01-09 10:27:29,961 WARNING [nectarchain.data.container.core] (core._container_from_hdf5): group ``/`` does not have a child named ``data``
2024-01-09 10:27:29,961 WARNING [nectarchain.data.container.core] (core._container_from_hdf5): group ``/`` does not have a child named ``data``
2024-01-09 10:27:29,961 WARNING [nectarchain.data.container.core] (core._container_from_hdf5): group ``/`` does not have a child named ``data``
2024-01-09 10:27:29,961 WARNING [nectarchain.data.container.core] (core._container_from_hdf5): group ``/`` does not have a child named ``data``
2024-01-09 10:27:29,962 WARNING [nectarchain.data.container.core] (core._container_from_hdf5): group ``/`` does not have a child named ``data``
2024-01-09 10:27:29,962 WARNING [nectarchain.data.container.core] (core._container_from_hdf5): group ``/`` does not have a child named ``data``
2024-01-09 10:27:29,962 WARNING [nectarchain.data.container.core] (core._container_from_hdf5): group ``/`` does not have a child named ``data``
2024-01-09 10:27:29,962 WARNING [nectarchain.data.container.core] (core._container_from_hdf5): group ``/`` does not have a child named ``data``
2024-01-09 10:27:29,963 - __main__ - WARNING - cannot access local variable 'merged_containers' where it is not associated with a value
Traceback (most recent call last):
  File "/home/jlenain/local/src/python/cta-observatory/nectarchain/src/nectarchain/user_scripts/ggrolleron/gain_SPEfit_computation.py", line 190, in main
    tool.start(figpath = _figpath,display = args.display)
  File "/home/jlenain/local/src/python/cta-observatory/nectarchain/src/nectarchain/makers/calibration/gain/FlatFieldSPEMakers.py", line 102, in start
    chargesContaienrs_merdes_along_slices = ArrayDataComponent.merge_along_slices(containers_generator=chargesContainers)
                                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/jlenain/local/src/python/cta-observatory/nectarchain/src/nectarchain/makers/component/core.py", line 289, in merge_along_slices
    return merged_containers
           ^^^^^^^^^^^^^^^^^
UnboundLocalError: cannot access local variable 'merged_containers' where it is not associated with a value
2024-01-09 10:27:29,977 - __main__ - INFO - time for execution is 3.10e+00 sec
Closing remaining open files:/data/users/jlenain/tmp/SPEfit/FlatFieldSPEHHVNectarCAM_run3942_maxevents100_GlobalPeakWindowSum_window_width_16_window_shift_4_sliced1000.h5...done

guillaume.grolleron added 3 commits January 12, 2024 11:20
-bugfix : check valididty of container before to write when
finishing component
-creation of your own component and tool
-SPE tools use
-exctraction of waveforms and chgares with tools and components.
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

guillaume.grolleron added 2 commits January 15, 2024 16:21
of the nectarchain module
-typo fix
-overwrite argument of tool better defined
Now the read of a container will always return a generator,
which can be ciomposed by only one item.
@guillaumegrolleron
Copy link
Contributor Author

Hi all,
The issue mentioned has been fixed, it was cause because of a method was expecting a generator while a no iterable object was returned. I fixed the issue in the commit [a233bb9].
3 tutorials has been implemented, one to explain how to write your own component, one to use the WaveformsNectarCAMCalibrationTool and ChargesNectarCAMCalibrationTool, and another one to use the SPE fit. This two tutorials are less complete than the one about de Component and Tool explanation.
I would say that this PR is ready to merge. I will bring documentation, unit test in a further PR.

@jlenain jlenain self-requested a review January 16, 2024 21:45
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.

Thank you so much, @guillaumegrolleron , this is a huge piece of work!
I just have a small request concerning ignoring Jupyter notebooks and enabling pre-commit hooks, in order to automaically convert them as python scripts before pushing them to the repo.
Thanks!

.gitignore Outdated Show resolved Hide resolved
@vmarandon
Copy link
Contributor

Thanks a lot for the hard work !

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 !
I made a last check re-running the SPE fit first extracting the waveforms from a fits.gz file, and running the SPE fits on the intermediate HDF5 file, all fine !
I am merging this PR now.

@jlenain jlenain merged commit 3579686 into cta-observatory:master Jan 22, 2024
12 checks passed
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.

3 participants