Skip to content

Muon fit improvement#2736

Merged
maxnoe merged 105 commits into
mainfrom
muon_fit_improvement
May 14, 2025
Merged

Muon fit improvement#2736
maxnoe merged 105 commits into
mainfrom
muon_fit_improvement

Conversation

@burmist-git
Copy link
Copy Markdown
Member

The muon ring fitter needs to be improved.
The existing toy tests for muons are overly simplified.
Realistic simulations reveal significant discrepancies between the true ring and the fitted result (see the attached PNG). The fitted parameters are shown in green.

Screenshot from 2025-04-09 16-55-44

The second major component will be fitting the muon impact point while accounting for the muon’s inclination.

I stated with :
Enhance the toy model for muon ring simulation by incorporating user-defined asymmetry in both the x and y directions.
Update the muon ring fitter test to include this ring asymmetry.

…defined asymmetry in both the x and y directions. Updating the muon ring fitter test to include ring asymmetry.
@mexanick mexanick self-requested a review April 9, 2025 15:02
@ctao-dpps-sonarqube

This comment has been minimized.

@maxnoe
Copy link
Copy Markdown
Member

maxnoe commented Apr 9, 2025

Could you please provide the code that produced your images?

I can't reproduce this systematic over-estimation of the radius locally.

@burmist-git
Copy link
Copy Markdown
Member Author

burmist-git commented Apr 10, 2025

@maxnoe It is very simple even with current test.
~/ctapipe/src/ctapipe/image/muon/tests/test_ring_fitter.py
Screenshot from 2025-04-10 08-57-30
just use the bigger ring:

center_xs = 0.2 * u.m
center_ys = 0.2 * u.m
radius = 0.6 * u.m
width = 0.1 * u.m

But in case of non symmetric ring you will get the error even with small ring.
For non symmetric ring you will need to take this branch :muon_fit_improvement
As well you can have a plotting script : https://github.com/burmist-git/ctapipe_run_dir/blob/master/muons_ana_ctapipe.ipynb
For the plot above I need to provide you much more than just the plotting script.

@maxnoe
Copy link
Copy Markdown
Member

maxnoe commented Apr 10, 2025

Could you please provide a minimal example that re-produces this error in a self contained script or test that can just be run?

@burmist-git
Copy link
Copy Markdown
Member Author

burmist-git commented Apr 10, 2025

Ah sure ... You will need this branch :muon_fit_improvement
https://github.com/burmist-git/ctapipe_run_dir/blob/master/test_ring_fitter.py
I can push the test script so it will fail the tests but I did not make it so the tests are passed.

To run it you will need :

(cta-dev) > python -m pytest -s ~test_ring_fitter.py

@burmist-git
Copy link
Copy Markdown
Member Author

Could you please provide a minimal example that re-produces this error in a self contained script or test that can just be run?

Did you reproduce the error ?

@maxnoe
Copy link
Copy Markdown
Member

maxnoe commented Apr 10, 2025

The test is passing for me:

❯ git status
On branch muon_fit_improvement
Your branch is up to date with 'origin/muon_fit_improvement'.

Untracked files:
  (use "git add <file>..." to include in what will be committed)
        fit_muon.py

nothing added to commit but untracked files present (use "git add" to track)
❯ pytest src/ctapipe/image/muon/tests/test_ring_fitter.py
================================================= test session starts ==================================================
platform linux -- Python 3.12.9, pytest-8.3.5, pluggy-1.5.0
Running tests in src/ctapipe/image/muon/tests/test_ring_fitter.py.

Date: 2025-04-10T15:07:36

Platform: Linux-5.15.167.4-microsoft-standard-WSL2-x86_64-with-glibc2.34

Executable: /home/maxnoe/.local/conda/envs/cta-dev/bin/python3.12

Full Python Version:
3.12.9 | packaged by conda-forge | (main, Mar  4 2025, 22:48:41) [GCC 13.3.0]

encodings: sys: utf-8, locale: UTF-8, filesystem: utf-8
byteorder: little
float info: dig: 15, mant_dig: 15

Package versions:
eventio: 1.15.0
numpy: 2.1.3
scipy: 1.15.2
astropy: 7.0.1
numba: 0.61.0


rootdir: /home/maxnoe/CTAO/ctapipe
configfile: pyproject.toml
plugins: astropy-header-0.2.2, cov-6.0.0, anyio-4.8.0, requirements-0.2.0
collected 3 items

src/ctapipe/image/muon/tests/test_ring_fitter.py ...                                                             [100%]

================================================== 3 passed in 0.55s ===================================================

@burmist-git
Copy link
Copy Markdown
Member Author

burmist-git commented Apr 10, 2025

Ok try now.
This ring is failing to fit

image

image

Comment thread src/ctapipe/image/toymodel.py Outdated
@maxnoe
Copy link
Copy Markdown
Member

maxnoe commented Apr 10, 2025

I think you can always find some specific example where a fit is not getting to the true answer within some tight tolerance.

The question I am trying to understand here is whether there is really an issue, or that is just the expected resolution of such a method.

Do you have any concrete evidence, that there is a bug? Or a proposal for an improvement?

@burmist-git
Copy link
Copy Markdown
Member Author

In the current state even a very good muons (momentum > 13 GeV and theta [0,0.1 deg]) and impact point about [0-13]~m are only 30 % efficient to fit the muon in a good way. The example of the ring i show is very much simple to be fitted correctly.

@maxnoe
Copy link
Copy Markdown
Member

maxnoe commented Apr 10, 2025

@burmist-git Ok, but have you identified a concrete issue? Do you propose to change the existing methods somehow? Add a new method?

@burmist-git
Copy link
Copy Markdown
Member Author

burmist-git commented Apr 10, 2025

@maxnoe Yes the non ideal muons (asymmetric in terms of pixel intensity) are not fitted well. For this reason i have implemented the test with intensity asymmetry.

The plan in very simple use the first method for defining the seed parameter and then fit with second method or similar one.

@maxnoe
Copy link
Copy Markdown
Member

maxnoe commented Apr 10, 2025

@burmist-git I think you misunderstand me: I am asking if you have identified a bug. The symptom I have now understood.

@burmist-git
Copy link
Copy Markdown
Member Author

Yes, the kundu_chaudhuri method is good but only in case of uniform intensities ring or part of the ring:

image

In reality one have always muons with some intensity non uniformity. It is function of muon inclination and muon impact point. So this is main reason why I included the intensity asymmetry into the toy simulation.

The taubin_circle_fit as well have two problems:

  1. It is using as initial parameters xc = yx= 0 and R = x_max / 2. And this have to be changed for the values obtained with kundu_chaudhuri methode.
  2. It fit the mask only (does not include the pixel intensities).

@burmist-git
Copy link
Copy Markdown
Member Author

burmist-git commented Apr 11, 2025

Some updates before end of the week :-). In addition the kundu_chaudhuri is very sensitive to noise.
Screenshot from 2025-04-11 17-20-17
Screenshot from 2025-04-11 17-23-32

So one needs to run it on the mask only. And use the obtained values as a seed for munuit fit.
Blue is true values and red is a kundu_chaudhuri estimation.

@maxnoe
Copy link
Copy Markdown
Member

maxnoe commented Apr 11, 2025

Sorry for asking again, I will try to express myself more clearly:

Do you think there there is an actual bug here, as in "A programming mistake"?

Or is it "just" that the correctly implemented current methods are not well-suited?

@burmist-git
Copy link
Copy Markdown
Member Author

The ring fitting procedure has no bugs. It is working correctly mean : doing exactly what it is asked to do. With little ajustement of the logic it will do much better.

Comment thread docs/changes/2480.bugfix.rst Outdated
…ing less asymmetrical still can be fitted with chaudhuri kundu method (not real fit). While the ring with much larger asymmetry can be fitted only with updated taubin circle fit and one more introduced methode. This newly introduced method uses the chaudhuri kundu formulas (on the mask) to get the approximate initial values of r, xc and yc and feed it to taubin circle fit.
…udhuri kundu formula to calculate the initial muon ring parameters and fead them to the taubin fit.
@burmist-git
Copy link
Copy Markdown
Member Author

The ring fitting procedure has been updated . I added the weight into the taubin fit. And added the ring fitting combined method (kundu_chaudhuri_taubin). This newly introduced method uses the chaudhuri kundu formulas (on the mask) to get the approximate initial values of r, xc and yc and feed it to taubin circle fit. The tests with much more distorted ring have been successfully passed. Note these more distorted rings (colosseum like asymmetry) could not be fitted correctly with old taubin fit without weights.

@burmist-git burmist-git marked this pull request as ready for review April 23, 2025 17:17
Comment thread src/ctapipe/image/muon/ring_fitter.py Outdated
maxnoe
maxnoe previously approved these changes May 14, 2025
@burmist-git
Copy link
Copy Markdown
Member Author

burmist-git commented May 14, 2025

@maxnoe @kosack @mexanick looks it is ready ?

@ctao-dpps-sonarqube

This comment has been minimized.

2 similar comments
@ctao-dpps-sonarqube

This comment has been minimized.

@ctao-dpps-sonarqube

This comment has been minimized.

mexanick
mexanick previously approved these changes May 14, 2025
Copy link
Copy Markdown
Contributor

@mexanick mexanick left a comment

Choose a reason for hiding this comment

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

LGTM. Not sure why CI jobs status is not yet reported, from the actions page it looks like it passed without issues.

@ctao-dpps-sonarqube

This comment has been minimized.

@maxnoe
Copy link
Copy Markdown
Member

maxnoe commented May 14, 2025

@mexanick No jobs ran for the latest commit here.

Comment thread src/ctapipe/image/toymodel.py Outdated
Co-authored-by: Maximilian Linhoff <maximilian.linhoff@cta-observatory.org>
@maxnoe
Copy link
Copy Markdown
Member

maxnoe commented May 14, 2025

Ok, with the latest push, jobs are passing

@ctao-dpps-sonarqube
Copy link
Copy Markdown

Passed

Analysis Details

0 Issues

  • Bug 0 Bugs
  • Vulnerability 0 Vulnerabilities
  • Code Smell 0 Code Smells

Coverage and Duplications

  • Coverage 93.30% Coverage (93.70% Estimated after merge)
  • Duplications 0.00% Duplicated Code (0.70% Estimated after merge)

Project ID: cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs

View in SonarQube

@mexanick mexanick self-requested a review May 14, 2025 13:10
@maxnoe maxnoe merged commit 6117878 into main May 14, 2025
13 checks passed
@maxnoe maxnoe deleted the muon_fit_improvement branch May 14, 2025 13:15
@maxnoe
Copy link
Copy Markdown
Member

maxnoe commented May 14, 2025

@burmist-git Thanks a lot for this contribution.

It would be nice if you could report somewhere (e.g. in an ASWG call) on the improvements on the muon analysis resulting from this change

@burmist-git
Copy link
Copy Markdown
Member Author

Sure, this needs to be done. However, there are many things coming up next month, including the next sprint and the ICRC.

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