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

Thermal tests + CI in DQM #122

Merged
merged 9 commits into from
Aug 22, 2024
Merged

Conversation

hashkar
Copy link
Collaborator

@hashkar hashkar commented Jul 3, 2024

This pull request contains changes to make the DQM easily usable for the thermal tests.
The main changes also include the integration of CI tests for the DQM.
This is just an initiation to the CI tests. They are not complete yet. The first function of each class is tested here.

@hashkar
Copy link
Collaborator Author

hashkar commented Jul 4, 2024

One thing that needs to be fixed before accepting this pull request is the path to the data file used for the tests. @jlenain can you point me to the one you mentioned yesterday?

@jlenain
Copy link
Collaborator

jlenain commented Jul 4, 2024

One thing that needs to be fixed before accepting this pull request is the path to the data file used for the tests. @jlenain can you point me to the one you mentioned yesterday?

There are a few such instances throughout nectarchain, e.g.

run_file = get_dataset_path('NectarCAM.Run3938.30events.fits.fz')

@jlenain
Copy link
Collaborator

jlenain commented Jul 4, 2024

@hashkar , one thing that I am wondering is whether the DQM module is the best place for thermal test analyses. In my understanding, the DQM should run a quality monitoring as generic as possible. A dedicated analysis of runs for thermal tests would be better placed into user_scripts, in my opinion. What do you think ? @frnbrun , @vmarandon as well ?

@hashkar
Copy link
Collaborator Author

hashkar commented Jul 4, 2024

@jlenain I totally agree. The DQM module is still the same I just changed small things in it to be able to compute the quantities needed for the thermal tests easily. These quantities are also useful for the DQM (for example pedestal averaged for the first 20 samples). In the user_script there will be the script to launch the thermal tests and analyze + plot the results.

@jlenain
Copy link
Collaborator

jlenain commented Jul 19, 2024

I guess the prints were meant to assess which value to assert the tests. Could you please remove these? Thanks!

@jlenain jlenain marked this pull request as draft July 26, 2024 09:38
@jlenain jlenain changed the title Thermal tests + CI Thermal tests + CI in DQM Jul 26, 2024
@hashkar hashkar marked this pull request as ready for review August 2, 2024 09:28
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.

Some changes may be redundant with PR #148 , or adjusted accordingly. We'll take care of that when merging the two PRs.

environment.yml Outdated Show resolved Hide resolved
src/nectarchain/dqm/__init__.py Show resolved Hide resolved
src/nectarchain/dqm/camera_monitoring.py Show resolved Hide resolved
src/nectarchain/dqm/charge_integration.py Show resolved Hide resolved
src/nectarchain/dqm/mean_waveforms.py Show resolved Hide resolved
Copy link

codecov bot commented Aug 22, 2024

Codecov Report

Attention: Patch coverage is 44.82759% with 32 lines in your changes missing coverage. Please review.

Project coverage is 48.63%. Comparing base (b306f30) to head (911ad40).
Report is 21 commits behind head on main.

Files Patch % Lines
src/nectarchain/dqm/charge_integration.py 34.09% 29 Missing ⚠️
src/nectarchain/dqm/start_dqm.py 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #122      +/-   ##
==========================================
+ Coverage   40.76%   48.63%   +7.86%     
==========================================
  Files          65       64       -1     
  Lines        4452     4581     +129     
==========================================
+ Hits         1815     2228     +413     
+ Misses       2637     2353     -284     

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

@jlenain jlenain merged commit 7554a70 into cta-observatory:main Aug 22, 2024
10 of 11 checks passed
@jlenain jlenain deleted the ThermalTests branch August 23, 2024 07:43
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