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

Add container/component/tool for pedestal estimation #115

Merged
merged 21 commits into from
Mar 29, 2024

Conversation

tibaldo
Copy link
Member

@tibaldo tibaldo commented Mar 21, 2024

This PR add a tool/component dedicated to estimate pedestals based on data that contain events of type SKYPEDESTAL. Pedestals are stored in a dedicated container following the nectarchain conventions (pixels_id etc.).
Several methods for filtering the data are available based on timestamps of the events, standard deviations of the waveforms and the charge distribution.
In order to reduce the memory load the tool is built in such a way that pedestals are estimated in slices with a number of events events_per_slice and then results combined at the end.
PS: I'm not sure why in the file comparison I see the renaming master-> main which is already integrated in the upstream repository

@jlenain
Copy link
Collaborator

jlenain commented Mar 22, 2024

CI tests are currently failling due to #116 . #117 should fix it.

* Activate the `pytest-xdist` addon for `pytest` in CI.

* Fix typo

---------

Co-authored-by: Jean-Philippe Lenain <jlenain@in2p3.fr>
Copy link

codecov bot commented Mar 22, 2024

Codecov Report

Attention: Patch coverage is 92.07650% with 29 lines in your changes are missing coverage. Please review.

Project coverage is 39.43%. Comparing base (51b976c) to head (1f09a0d).

Files Patch % Lines
src/nectarchain/makers/core.py 26.66% 11 Missing ⚠️
...c/nectarchain/makers/calibration/pedestalMakers.py 85.93% 9 Missing ⚠️
.../nectarchain/makers/component/PedestalComponent.py 93.38% 9 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #115       +/-   ##
===========================================
+ Coverage   28.54%   39.43%   +10.88%     
===========================================
  Files          62       65        +3     
  Lines        4102     4453      +351     
===========================================
+ Hits         1171     1756      +585     
+ Misses       2931     2697      -234     

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

@jlenain jlenain requested review from jlenain and vmarandon March 22, 2024 08:57
@jlenain
Copy link
Collaborator

jlenain commented Mar 22, 2024

PS: I'm not sure why in the file comparison I see the renaming master-> main which is already integrated in the upstream repository

I think it is, maybe, because you pull from the main branch of the repo into your dev branch before pushing your commits into it ?
If so, new features introduced in the main are duplicated in your dev branch. This should not be any issue at merge (we'll clean the commit log), but the proper workflow is to ignore any new commit from the main in your dev branch. Any potential conflicts with the main will be flagged, and can be fixed, when we merge the PR.

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, @tibaldo !
I tested your branch locally, and everything is running very smoothly.
To me, the PR is in an excellent shape and is ready to be merged.

@jlenain jlenain merged commit b306f30 into cta-observatory:main Mar 29, 2024
10 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