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

Dvr pixel selector #1089

Merged
merged 38 commits into from
May 14, 2023
Merged

Dvr pixel selector #1089

merged 38 commits into from
May 14, 2023

Conversation

moralejo
Copy link
Collaborator

@moralejo moralejo commented May 2, 2023

This scripts uses DL1 files to identify (event-wise) pixels likely containing a signal, and creates hdf5 files with this information for use in the raw data volume reduction.

There are no tests for this script as of now, it requires to have DL1 files with the DL1a images. Are they produced? At the end of the tests the created DL1 files only have DL1b.

@moralejo moralejo marked this pull request as ready for review May 3, 2023 07:06
@gabemery
Copy link
Collaborator

gabemery commented May 3, 2023

There are no tests for this script as of now, it requires to have DL1 files with the DL1a images. Are they produced? At the end of the tests the created DL1 files only have DL1b.

This test seems to write data

def test_r0_to_dl1_observed(tmp_path):

You may be able to chain your tests after this one.

I am not convinced by the current script usage. Running twice the same script, the second time using the output of the first and adding a flag feel non intuitive. Maybe use a "processing_step" input parameter which can be 'settings', 'pixelmask' or 'both' depending on if you want to run step one, two or both directly?

@moralejo
Copy link
Collaborator Author

moralejo commented May 3, 2023

Thanks @gabemery . Now the script is steered in this way:

lstchain_dvr_pixselector --action compute_dvr_settings ... (which is the default)

or

lstchain_dvr_pixselector --action create_pixel_masks ...

I think it is more clear in this way.
(I did not add the "both" option, which would be somewhat complicated to implement)

As for the test, I will try, thanks!

lstchain/scripts/lstchain_dvr_pixselector.py Show resolved Hide resolved
lstchain/scripts/lstchain_dvr_pixselector.py Show resolved Hide resolved
lstchain/scripts/lstchain_dvr_pixselector.py Show resolved Hide resolved
lstchain/scripts/lstchain_dvr_pixselector.py Show resolved Hide resolved
lstchain/scripts/lstchain_dvr_pixselector.py Outdated Show resolved Hide resolved
lstchain/scripts/lstchain_dvr_pixselector.py Outdated Show resolved Hide resolved
lstchain/scripts/lstchain_dvr_pixselector.py Show resolved Hide resolved
lstchain/scripts/lstchain_dvr_pixselector.py Outdated Show resolved Hide resolved
lstchain/scripts/lstchain_dvr_pixselector.py Outdated Show resolved Hide resolved
lstchain/scripts/lstchain_dvr_pixselector.py Show resolved Hide resolved
@moralejo moralejo marked this pull request as draft May 4, 2023 17:54
@moralejo moralejo force-pushed the dvr_pixel_selector branch from 2edf6aa to f941565 Compare May 4, 2023 18:05
@codecov
Copy link

codecov bot commented May 4, 2023

Codecov Report

Patch coverage: 91.49% and project coverage change: +0.35 🎉

Comparison is base (8fb5cb5) 74.02% compared to head (94b1a8e) 74.38%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1089      +/-   ##
==========================================
+ Coverage   74.02%   74.38%   +0.35%     
==========================================
  Files         123      124       +1     
  Lines       11869    12116     +247     
==========================================
+ Hits         8786     9012     +226     
- Misses       3083     3104      +21     
Impacted Files Coverage Δ
lstchain/scripts/lstchain_dvr_pixselector.py 90.94% <90.94%> (ø)
lstchain/conftest.py 97.65% <100.00%> (+0.07%) ⬆️
lstchain/scripts/tests/test_lstchain_scripts.py 99.66% <100.00%> (+0.01%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

moralejo added 23 commits May 9, 2023 14:18
survival rate to determine the charge threshold to be applied.
Test of pixel survival now done on pedestal events, not cosmics
Raised threshold (for higher NSB) no longer based on charge_per_mil_pedestal
the data volume reduction settings without creating the files containing
the pixel masks for the actual reduction.
@moralejo moralejo force-pushed the dvr_pixel_selector branch from f941565 to 285768e Compare May 9, 2023 13:20
@moralejo moralejo marked this pull request as ready for review May 9, 2023 13:20
@moralejo moralejo requested a review from rlopezcoto May 9, 2023 13:20
@moralejo
Copy link
Collaborator Author

moralejo commented May 9, 2023

All ready from my side.

@moralejo moralejo merged commit 58a12a3 into master May 14, 2023
@moralejo moralejo deleted the dvr_pixel_selector branch May 14, 2023 22:48
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.

4 participants