-
Notifications
You must be signed in to change notification settings - Fork 77
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
Update dl1 to dl2 script (Less required memory and less required step) #1224
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1224 +/- ##
==========================================
+ Coverage 72.67% 72.87% +0.19%
==========================================
Files 132 133 +1
Lines 13655 13825 +170
==========================================
+ Hits 9924 10075 +151
- Misses 3731 3750 +19 ☔ View full report in Codecov by Sentry. |
lstchain/reco/dl1_to_dl2.py
Outdated
@@ -731,6 +730,9 @@ def apply_models(dl1, | |||
|
|||
# gammaness is the prediction probability for the first class (0) | |||
dl2['gammaness'] = probs[:, 0] | |||
col = list(classifier.classes_).index(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice that you get the col automtaically.. but the line above still uses the hard-coded index
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SeiyaNozaki could you move line 733 up one line and use col in 732?
After that we can merge I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry I forgot to implement @maxnoe's comment.. now implemented!
Concerning the first point, it was on purpose for multiple input files... So I will modify codes like
|
I realized I didn't read carefully comments from @gabemery during the school... sorry! |
lstchain/reco/dl1_to_dl2.py
Outdated
@@ -731,6 +730,9 @@ def apply_models(dl1, | |||
|
|||
# gammaness is the prediction probability for the first class (0) | |||
dl2['gammaness'] = probs[:, 0] | |||
col = list(classifier.classes_).index(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SeiyaNozaki could you move line 733 up one line and use col in 732?
After that we can merge I think
This PR updates the dl1 to dl2 step:
apply_models
. This option was already implemented inapply_models
, but not used for the script before.gammaness
andreco_type
. Butreco_type
can be set based ongammaness
as suggested by @maxnoe here. Even, we can removereco_type
which will not be used for the analysis.Extra srcdep parameters and limit the offset of gamma MC for training #1077 (comment)