-
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 muon_analysis.py #1112
Update muon_analysis.py #1112
Conversation
With these changes, the fit_muon function now fits a ring correctly even under high noise conditions. This was previously not the case. The changes made are: the min. number of pixel neighbors to survive the tailcuts cleaning is now set to 2 (previously was set to default, which is 0). If no tailcuts are indicated as an input, the function will select appropiate tailcuts based on the noise of the image. Also, in the analyze_muon_event function, when calling the fit_muon function in line 231, the 'tailcuts' input has been removed. This way, when using analyze_muon_event, the tailcuts will be adapted to the image's noise.
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.
Did you check that the fits give similar results to those we are getting at the moment?
Also, I am not sure that
-If no tailcuts are indicated as an input, the function will select appropiate tailcuts based on the noise of the image.
is exactly the way to go since we may be removing too many pixels for noisy images, which could affect the total charge extracted. @moralejo did you take that into account when you reviewed this PR?
lstchain/image/muon/muon_analysis.py
Outdated
negative_Q = np.sort(image[image <= 0]) | ||
|
||
hist, bins = np.histogram(negative_Q,range=(-15,0),bins=30) | ||
bins=bins[:-1] |
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.
comment from #1012 (comment)
lstchain/image/muon/muon_analysis.py
Outdated
|
||
idx = (np.abs(normalised_cumulative_hist - 0.318)).argmin() #Find q closest to standard deviation | ||
dev = np.abs(bins[idx]) | ||
#MORALEJO: we want to get, from a single image, a quantity related to the width of the noise distribution, |
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.
this comment can probably be edited rather than copy/paste that of:
#1012 (comment)
Are these changes still valid? If so, we should try to finish this PR. |
To add some context: this was done as a summer student project, but time was too short to finalize it.
This will affect only the geometrical ring fit. Only through the ring parameters can the charge-related parameters (computed later) be modified. And they are computed in a way (i.e. rather wide band around the ring) that they should be rather robust against small changes of ring parameters. if 4-sigma, 2-sigma tail cuts (which should correspond to ~7.0, 3.5 pe for very dark data) remove a significant part of the ring charge then the ring is too dim for proper analysis anyway. It may be the case for 10 * darkNSB, which is our limit of operation... On the other hand, muon calibration under such conditions may not be absolutely needed (one can check the calibration from close-in-time data with less NSB) |
The original idea of this was, "if we can clearly see the rings on a single image with high NSB (for which nevertheless the standard ring analysis failed completely), and distinguish it clearly from the noise, then it should also be possible to obtain the ring parameters using only the info contained in the event". |
BTW what would be the correct way to merge the (many) change in the main branch into this one? I tried, but there are lots of conflicts which I think should not be there (seem totally unrelated to the only file that was changed in this branch) |
This branch doesn't have any conflicts with main and can be merged as is. If you want to update this branch before merging, you can merge or rebase This just works and doesn't create conflicts:
As does rebasing:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1112 +/- ##
==========================================
+ Coverage 73.49% 73.51% +0.02%
==========================================
Files 134 134
Lines 14207 14218 +11
==========================================
+ Hits 10442 10453 +11
Misses 3765 3765 ☔ View full report in Codecov by Sentry. |
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.
It looks good to me
Co-authored-by: Daniel Morcuende <dmorcuende@iaa.es>
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.
looks good to me
With these changes, the fit_muon function now fits a ring correctly even under high noise conditions. This was previously not the case. The changes made are:
-the min. number of pixel neighbors to survive the tailcuts cleaning is now set to 2 (previously was set to default, which is 0). -If no tailcuts are indicated as an input, the function will select appropiate tailcuts based on the noise of the image.
Also, in the analyze_muon_event function, when calling the fit_muon function in line 231, the 'tailcuts' input has been removed. This way, when using analyze_muon_event, the tailcuts will be adapted to the image's noise.