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

Pixel participation #81

Merged
merged 9 commits into from
Feb 9, 2024

Conversation

hashkar
Copy link
Collaborator

@hashkar hashkar commented Oct 30, 2023

Adding pixel participation fraction and BPX timeline modules to the DQM - PRELIMINARY WORKING VERSION

@jlenain
Copy link
Collaborator

jlenain commented Oct 30, 2023

Hi @hashkar !
Thank you! Could you first rebase your branch, to accommodate for the recent #78 and #74 ?
For instance, start_calib.py was renamed start_dqm.py.
Thanks!

@jlenain jlenain changed the title Ashkar pixel participation Pixel participation Nov 6, 2023
src/nectarchain/dqm/dqm_summary_processor.py Outdated Show resolved Hide resolved
src/nectarchain/dqm/start_dqm.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Dec 6, 2023

Codecov Report

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

Comparison is base (a31bad2) 30.92% compared to head (dc51915) 28.69%.
Report is 5 commits behind head on master.

Files Patch % Lines
src/nectarchain/dqm/pixel_timeline.py 0.00% 94 Missing ⚠️
src/nectarchain/dqm/pixel_participation.py 0.00% 93 Missing ⚠️
src/nectarchain/dqm/start_dqm.py 0.00% 32 Missing ⚠️
src/nectarchain/dqm/charge_integration.py 0.00% 27 Missing ⚠️
src/nectarchain/dqm/mean_camera_display.py 0.00% 27 Missing ⚠️
src/nectarchain/dqm/camera_monitoring.py 0.00% 24 Missing ⚠️
src/nectarchain/dqm/trigger_statistics.py 0.00% 19 Missing ⚠️
src/nectarchain/dqm/dqm_summary_processor.py 0.00% 13 Missing ⚠️
src/nectarchain/dqm/mean_waveforms.py 0.00% 13 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #81      +/-   ##
==========================================
- Coverage   30.92%   28.69%   -2.23%     
==========================================
  Files          60       62       +2     
  Lines        3754     4049     +295     
==========================================
+ Hits         1161     1162       +1     
- Misses       2593     2887     +294     

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

 Please enter the commit message for your changes. Lines starting
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.

Hi @hashkar ,
Thanks for the changes. There are a couple of coding conventions should be adhered to, before merging this PR.

src/nectarchain/dqm/pixel_participation.py Outdated Show resolved Hide resolved
src/nectarchain/dqm/pixel_participation.py Outdated Show resolved Hide resolved
src/nectarchain/dqm/pixel_participation.py Outdated Show resolved Hide resolved
src/nectarchain/dqm/pixel_participation.py Show resolved Hide resolved
src/nectarchain/dqm/pixel_participation.py Outdated Show resolved Hide resolved
src/nectarchain/dqm/pixel_timeline.py Outdated Show resolved Hide resolved
src/nectarchain/dqm/pixel_timeline.py Show resolved Hide resolved
src/nectarchain/dqm/pixel_timeline.py Show resolved Hide resolved
src/nectarchain/dqm/pixel_timeline.py Show resolved Hide resolved
src/nectarchain/dqm/pixel_timeline.py Outdated Show resolved Hide resolved
@jlenain
Copy link
Collaborator

jlenain commented Dec 12, 2023

In the meantime, I was able to test this PR on e.g. run 2943. Overall, it took almost 9h to process a single run, which is far too long, in my opinion. The DQM code would require a thorough optimization.

Some resulting plots from this PR are:

Screenshot from 2023-12-12 17-51-20
Screenshot from 2023-12-12 17-51-16
Screenshot from 2023-12-12 17-51-05
Screenshot from 2023-12-12 17-51-00
Screenshot from 2023-12-12 17-50-46
Screenshot from 2023-12-12 17-50-36

@vmarandon , would you have some suggestions to make on them ?

@hashkar
Copy link
Collaborator Author

hashkar commented Dec 12, 2023

@jlenain 9 hours for 1 run seems way too much. When I run the DQM on my local machine I can process a run (single file) of ~8000 events in a few minutes with the plots. I usually run it on 3 or 4 different runs before doing a commit. It takes less time if the plots are not computed. There is no doubt that the DQM needs optimization but are we sure that the 9 hours are due to the DQM code? if I am not mistaken your run has ~20 000 events which is not that much more. Maybe the plots are not well handled on a server? I know that the charge spectrum plots in charge_integration.py take a while to compute but still, we are talking a few minutes here.

@hashkar
Copy link
Collaborator Author

hashkar commented Feb 9, 2024

@jlenain this PR is ready for review now

@jlenain
Copy link
Collaborator

jlenain commented Feb 9, 2024

Support for python 3.8 in protozfits has been dropped 2 days ago (see https://gitlab.cta-observatory.org/cta-computing/common/protozfits-python/-/commit/bf475ba48ecb3a443023ef667f409d588af17fc6). Merging anyway.

@jlenain jlenain merged commit 8cc2103 into cta-observatory:master Feb 9, 2024
8 of 12 checks passed
@jlenain jlenain deleted the ashkar_pixel_participation branch February 9, 2024 13:44
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