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

Update muon_analysis.py #1112

Merged
merged 10 commits into from
Oct 7, 2024
Merged

Update muon_analysis.py #1112

merged 10 commits into from
Oct 7, 2024

Conversation

ferxinii
Copy link
Collaborator

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.

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.
Copy link
Contributor

@rlopezcoto rlopezcoto left a 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?

negative_Q = np.sort(image[image <= 0])

hist, bins = np.histogram(negative_Q,range=(-15,0),bins=30)
bins=bins[:-1]
Copy link
Contributor

Choose a reason for hiding this comment

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

comment from #1012 (comment)


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,
Copy link
Contributor

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)

@rlopezcoto rlopezcoto mentioned this pull request May 25, 2023
@morcuended
Copy link
Member

Are these changes still valid? If so, we should try to finish this PR.

@moralejo
Copy link
Collaborator

moralejo commented Sep 3, 2024

To add some context: this was done as a summer student project, but time was too short to finalize it.

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?

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)

@moralejo
Copy link
Collaborator

moralejo commented Sep 3, 2024

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".

@moralejo
Copy link
Collaborator

moralejo commented Sep 3, 2024

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)

@maxnoe
Copy link
Member

maxnoe commented Sep 3, 2024

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:

❯ git fetch
❯ git switch muon_analysis
Switched to branch 'muon_analysis'
Your branch is up to date with 'origin/muon_analysis'.
❯ git merge origin/main

As does rebasing:

❯ git fetch
❯ git switch muon_analysis
❯ git rebase origin/main

Copy link

codecov bot commented Sep 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.51%. Comparing base (378a4c3) to head (70a0a76).
Report is 23 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

morcuended
morcuended previously approved these changes Sep 17, 2024
Copy link
Member

@morcuended morcuended left a 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

lstchain/image/muon/muon_analysis.py Outdated Show resolved Hide resolved
Co-authored-by: Daniel Morcuende <dmorcuende@iaa.es>
Copy link
Member

@morcuended morcuended left a 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

@moralejo moralejo merged commit 620d0a9 into main Oct 7, 2024
9 checks passed
@moralejo moralejo deleted the muon_analysis branch October 7, 2024 10:49
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.

5 participants