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

Refactoring: Create LST Cleaning Component #1013

Closed
wants to merge 49 commits into from
Closed

Refactoring: Create LST Cleaning Component #1013

wants to merge 49 commits into from

Conversation

Hckjs
Copy link
Collaborator

@Hckjs Hckjs commented Aug 12, 2022

Referring to #972 and #667, I started refactoring the cleaning by creating a cleaning component which substitutes the cleaning parts in the r0_to_dl1 and dl1ab script.

Main changes:

  • There is now the LSTImageCleaner-class with a lot of traits for all the cleaning params and configurable by the config system
  • It basically has 3 steps: 1) get pedestal thresholds for tailcuts_clean 2) tailcuts_clean 3) lst_image_cleaning
  • the lst_image_cleaning consist of the steps apply_time_delta_cleaning, apply_dynamic_cleaning and selecting the largest_island which can be all turned on/off by the config system
  • instead of using the custom code for getting the main/largest island, i switched to the ctapipe function ctapipe.image.morphology.largest_island and renamed it to largest_island because main_island could be a bit ambiguous (main island could be largest or brightest one)
  • the standard_config is adapted to the LSTImageCleaner
  • the dl1ab-script does not use the LSTImageCleaner yet because it loops over the tables directly via pytables instead of using the EventSource (Want to switch that in a later PR). So the pedestal_cleaning part inside the LSTImageCleaner is not yet used by any script.

Concerns/Thoughts:

  • For now, i put the pedestal_cleaning part (i.e. calculating the pedestal thresholds for the tailcuts cleaning) in the call function of the LSTImageCleaner. But that would mean, that the thresholds are calculated again for every image which is a wasted effort (since it is calculated just once per run). But its not yet used as explained in the last bullet point above. So maybe its better to outsource the calculation of the pedestal thresholds outside the eventloop and pass them then to the LSTImageCleaner? idk maybe someone has a good solution for that ...

Hckjs added 30 commits July 7, 2022 16:41
- Add new LSTImageCleaner class
 - Adapt config handling
 - return also n_pixels in LSTImageCleaner for dl1.container in r0_to_dl1
- Missing if clause create some trouble
- Use Telescope Parameters the right way
- Forgot to add tel_ids to cleaning parameters in the if clauses...
- Fix also little typo
- Fix also an issue with the get_only_main_island function
- Add it directly to the LSTImageCleaner
- Update Docstrings
Hckjs added 17 commits July 19, 2022 10:59
- Add an additional function to the `LSTImageCleaner` to get a bit clearer
- Add a few more docstrings
- Adapt to r0_to_dl1 function
- Add a new function to get the pedestal values right in the event loop for he pedestal cleaning
- The dlab-script now uses hte lst_image_cleaning function
- Adapt also the `get_cleaning_params`-function
- Instead of creating an own function, use the ctapipe `largest_island` one
- Save few line in dl1ab script
- Log info was 1 line too early
- Dont raise an error now, when pedestal cleaning is set to true, but no pedestal values
have already been  processed in the r0_to_dl1 script
- Instead just skip the pedestal cleaning part in the ImageCleaner
- Dont have to use different configs now for r0_to_dl1 script and dl1ab script
@codecov
Copy link

codecov bot commented Aug 12, 2022

Codecov Report

Merging #1013 (7e1bf17) into master (ef53179) will increase coverage by 1.69%.
The diff coverage is 91.90%.

❗ Current head 7e1bf17 differs from pull request most recent head 9e0cde0. Consider uploading reports for the commit 9e0cde0 to get more accurate results

@@            Coverage Diff             @@
##           master    #1013      +/-   ##
==========================================
+ Coverage   71.54%   73.23%   +1.69%     
==========================================
  Files         118      119       +1     
  Lines       11251    11308      +57     
==========================================
+ Hits         8049     8281     +232     
+ Misses       3202     3027     -175     
Impacted Files Coverage Δ
lstchain/scripts/lstchain_dl1ab.py 80.74% <75.00%> (-1.19%) ⬇️
...stchain/calib/camera/pixel_threshold_estimation.py 76.27% <77.27%> (+48.01%) ⬆️
lstchain/reco/r0_to_dl1.py 93.41% <90.43%> (+0.72%) ⬆️
lstchain/image/cleaning.py 96.07% <95.23%> (-3.93%) ⬇️
...ib/camera/tests/test_pixel_threshold_estimation.py 100.00% <100.00%> (ø)
lstchain/image/tests/test_image.py 100.00% <100.00%> (ø)
lstchain/io/config.py 94.87% <100.00%> (ø)
lstchain/io/tests/test_config.py 100.00% <100.00%> (ø)
lstchain/scripts/tests/test_lstchain_scripts.py 99.64% <100.00%> (ø)
lstchain/datachecks/dl1_checker.py 75.73% <0.00%> (+4.50%) ⬆️
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Hckjs Hckjs closed this Apr 4, 2024
@Hckjs Hckjs deleted the cleaning_comp branch April 12, 2024 10:08
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.

1 participant