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

Refactoring of the calibration and container module #78

Merged
merged 62 commits into from
Oct 26, 2023

Conversation

guillaumegrolleron
Copy link
Contributor

I have been working last couple of days on the refactoring of the calibration code. As discussed with @jlenain and @vmarandon, we would like to have one module (makers) to make computations on raw data and give the calibrations coefficient (or whatever). Then the other modules should use the makers and all the computation part would be in the same place.
In this PR, I tried to achieve that goal. The container module has been completely rebuilt. Now the ChargesContainers and WaveformsContainers are only made to store data. This classes inherit from the ctapipe.container . The computation part which was previously in the container module has been moved in the makers module into two classes, the WaveformsMaker which is basically looping through an EventSource and stacking events informations and waveforms into a WaveformsContainers. The ChargesMaker do the same but extracting the charge instead of the waveforms.
The calibration part is now into the makers module. The SPE fit classes have been rebuilt to speed up the process.
I also started to write unit tests, but the coverage is not complete.

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
@codecov
Copy link

codecov bot commented Sep 19, 2023

Codecov Report

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

Comparison is base (0c65774) 3.08% compared to head (3683819) 33.18%.
Report is 3 commits behind head on master.

❗ Current head 3683819 differs from pull request most recent head ecaa250. Consider uploading reports for the commit ecaa250 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           master      #78       +/-   ##
===========================================
+ Coverage    3.08%   33.18%   +30.10%     
===========================================
  Files          36       47       +11     
  Lines        3571     3128      -443     
===========================================
+ Hits          110     1038      +928     
+ Misses       3461     2090     -1371     
Files Coverage Δ
src/nectarchain/__init__.py 100.00% <100.00%> (ø)
src/nectarchain/data/__init__.py 100.00% <100.00%> (ø)
src/nectarchain/data/container/__init__.py 100.00% <100.00%> (ø)
src/nectarchain/data/container/core.py 100.00% <100.00%> (ø)
src/nectarchain/makers/__init__.py 100.00% <100.00%> (ø)
src/nectarchain/makers/calibration/__init__.py 100.00% <100.00%> (ø)
src/nectarchain/makers/calibration/core.py 100.00% <100.00%> (ø)
...rc/nectarchain/makers/calibration/gain/__init__.py 100.00% <100.00%> (ø)
...tarchain/makers/calibration/gain/utils/__init__.py 100.00% <100.00%> (ø)
src/nectarchain/display/__init__.py 0.00% <0.00%> (ø)
... and 19 more

... and 3 files with indirect coverage changes

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

@guillaumegrolleron
Copy link
Contributor Author

guillaumegrolleron commented Sep 21, 2023

I all,
I removed in pytest the tests which need run files to be performed.
But I have been looking into lstchain, and they use a (script) to download from an external server with a password provided to the repository through the GitHub secrets feature. I guess we could easily do the same.

 Tool and Components framework
guillaume.grolleron added 4 commits October 23, 2023 14:32
    -> now we will use ctapipe HDF5TableWriter and Reader
-creation of recursive containers
makers with the ctapipe.Tool framework
-implementation of the NectarCAMComponent, the base class for nectrachain components,
- implementation of the ChargesComponent and WaveformsComponent
or waveforms using Tool and Component classes
@jlenain
Copy link
Collaborator

jlenain commented Oct 26, 2023

Thanks a lot, @guillaumegrolleron , for all the work.
I would propose to merge everything, and we will learn altogether how to use the ctapipe components.
Let me first resolve the current conflicts with master (meaning, first deleting src/nectarchain/calibration/container/charge.py and src/nectarchain/calibration/container/waveforms.py before merging).

jlenain added a commit that referenced this pull request Oct 26, 2023
@jlenain jlenain merged commit 1dc96fb into cta-observatory:master Oct 26, 2023
@jlenain
Copy link
Collaborator

jlenain commented Oct 26, 2023

While linting the code introduced in this PR via #74 , it seems some parts of @guillaumegrolleron 's refactoring of the code were not yet included in this PR.

I am very sorry for this, it seems we merged this PR a bit too fast.
Nevermind, let's have the rest in a new PR.

Pinging @guillaumegrolleron and @vmarandon .

jlenain added a commit to jlenain/nectarchain that referenced this pull request Oct 26, 2023
jlenain added a commit that referenced this pull request Oct 27, 2023
* Replace notebooks by percent python scripts converted with jupytext

* Clean examples directory

* Lint code

* Lint code

* Lint code

* Lint code

* Lint DQM

* Fix bug

* Further linting after merging PR #78

* Disable some unit tests

* Disable some unit tests

* Try again, disable some unit tests

* Try again, disable some unit tests

---------

Co-authored-by: jlenain <[email protected]>
@jlenain jlenain mentioned this pull request Oct 30, 2023
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.

2 participants