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

Estimate cleaning threshold with CatB pedestals if availables #1143

Merged
merged 10 commits into from
Nov 30, 2023

Conversation

FrancaCassol
Copy link
Collaborator

No description provided.

@codecov
Copy link

codecov bot commented Jul 21, 2023

Codecov Report

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

Comparison is base (9f8627a) 74.32% compared to head (3512db4) 74.27%.

Files Patch % Lines
lstchain/scripts/lstchain_dl1ab.py 0.00% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1143      +/-   ##
==========================================
- Coverage   74.32%   74.27%   -0.05%     
==========================================
  Files         130      130              
  Lines       13245    13253       +8     
==========================================
  Hits         9844     9844              
- Misses       3401     3409       +8     

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

lstchain/io/io.py Outdated Show resolved Hide resolved
@FrancaCassol FrancaCassol marked this pull request as ready for review November 2, 2023 10:28
@FrancaCassol
Copy link
Collaborator Author

Hi @maxnoe,
If nothing against could we merge this request?

Copy link
Member

@morcuended morcuended left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @FrancaCassol. I left a few comments. Is it possible to implement a test for this?

lstchain/io/io.py Outdated Show resolved Hide resolved
lstchain/scripts/lstchain_dl1ab.py Outdated Show resolved Hide resolved
lstchain/scripts/lstchain_dl1ab.py Outdated Show resolved Hide resolved
lstchain/scripts/lstchain_dl1ab.py Show resolved Hide resolved
@FrancaCassol
Copy link
Collaborator Author

Hi @morcuended, for implementing a test, we need first to merge PR #1147, in order to easily produce a catB calibration file.

morcuended
morcuended previously approved these changes Nov 3, 2023
@rlopezcoto
Copy link
Contributor

@FrancaCassol I resolved the conflicts, can you check if everything is alright?

@FrancaCassol
Copy link
Collaborator Author

Hi @rlopezcoto,
perhaps in order to be coherent, we should keep the "C" of CatB, uppercase or lowercase in the key name and in the key path:

dl1_mon_tel_catB_ped_key = "/dl1/monitoring/telescope/catB/pedestal"
dl1_mon_tel_catB_cal_key = "/dl1/monitoring/telescope/catB/calibration"
dl1_mon_tel_catB_flat_key = "/dl1/monitoring/telescope/catB/flatfield"

or

dl1_mon_tel_CatB_ped_key = "/dl1/monitoring/telescope/CatB/pedestal"
dl1_mon_tel_CatB_cal_key = "/dl1/monitoring/telescope/CatB/calibration"
dl1_mon_tel_CatB_flat_key = "/dl1/monitoring/telescope/CatB/flatfield"

@morcuended
Copy link
Member

in the hdf5 files merging script is still written as dl1_mon_tel_CatB_ped_key

@rlopezcoto
Copy link
Contributor

in the hdf5 files merging script is still written as dl1_mon_tel_CatB_ped_key

corrected now

Copy link
Member

@morcuended morcuended left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a minor comment that can be ignored. Looks good to me

Comment on lines +194 to +197
if args.catB_calibration_file is not None:
catB_pedestal_mean = np.array(catB_pedestal["charge_mean"])
catB_pedestal_std= np.array(catB_pedestal["charge_std"])
catB_threshold_clean_pe = catB_pedestal_mean + sigma * catB_pedestal_std
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe these lines could be somehow integrated inside get_threshold_from_dl1_file, which I think does something similar.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that they could come in a function, but I think it would be better to have a different one because there will be cases in which you may want tio extract catB parameters instead of the regular ones, right?
In any case, I leave @FrancaCassol to decide if it is a good idea to put a pin on this for the future

@rlopezcoto rlopezcoto merged commit a59c1a7 into main Nov 30, 2023
7 of 9 checks passed
@rlopezcoto rlopezcoto deleted the cleaning_with_CatB branch November 30, 2023 14: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.

5 participants