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

Likelihood reconstruction update #1200

Merged
merged 6 commits into from
Mar 12, 2024

Conversation

gabemery
Copy link
Collaborator

@gabemery gabemery commented Jan 22, 2024

This pull request covers 4 small changes:

  1. Adds the extraction of the pedestal variance used in the likelihood function from interleaved events. This was already covered in Use interleaved events to get the pedestal std deviation in the likelihood reco #1124 but not merged at the time. This is more stable than the current, event-wise extraction and allows to account for pixel differences. It is also needed for data after DVR.
  2. Clean up a test and log message from when the code needed to be pre-compiled
  3. Add support of source dependent analysis with the likelihood reconstruction
  4. Remove the 'weight' argument. It was tested during development but is not used anymore.

All of these changes were already tested extensively in a modified version of lstchain 0.9.8

I also checked that it run successfully up to the creation of DL2 files in the current version on small v0.10 (no DVR) datasets (without in depth check of the file content). The updated config file fails during training due to #1206

Copy link

codecov bot commented Jan 22, 2024

Codecov Report

Attention: Patch coverage is 32.00000% with 34 lines in your changes are missing coverage. Please review.

Project coverage is 72.81%. Comparing base (1d98657) to head (c241816).

Files Patch % Lines
lstchain/reco/reconstructor.py 32.00% 17 Missing ⚠️
lstchain/reco/dl1_to_dl2.py 33.33% 6 Missing ⚠️
lstchain/reco/r0_to_dl1.py 20.00% 4 Missing ⚠️
lstchain/scripts/lstchain_dl1_to_dl2.py 33.33% 4 Missing ⚠️
lstchain/reco/reconstructorCC.py 40.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1200      +/-   ##
==========================================
- Coverage   72.92%   72.81%   -0.12%     
==========================================
  Files         133      133              
  Lines       13885    13911      +26     
==========================================
+ Hits        10126    10129       +3     
- Misses       3759     3782      +23     

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

@gabemery gabemery self-assigned this Jan 24, 2024
@gabemery gabemery marked this pull request as ready for review January 24, 2024 14:32
Copy link
Collaborator

@SeiyaNozaki SeiyaNozaki left a comment

Choose a reason for hiding this comment

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

Hi @gabemery I reviewed especially the part related to source-dependent analysis. I put the comments there

lstchain/reco/dl1_to_dl2.py Outdated Show resolved Hide resolved
…_gradient and skewness even when the likelihood reconstruction is active
SeiyaNozaki
SeiyaNozaki previously approved these changes Mar 4, 2024
Copy link
Collaborator

@SeiyaNozaki SeiyaNozaki left a comment

Choose a reason for hiding this comment

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

Thanks for the answers @gabemery
I approved the change of codes related to source-dependent analysis

chaimain
chaimain previously approved these changes Mar 12, 2024
@gabemery gabemery dismissed stale reviews from chaimain and SeiyaNozaki via c241816 March 12, 2024 10:37
@rlopezcoto rlopezcoto merged commit e0f6d78 into main Mar 12, 2024
7 of 9 checks passed
@rlopezcoto rlopezcoto deleted the likelihood_reco_dvr_and_srcdep_update branch March 12, 2024 14:20
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