add Taubin fit #1154

momorning · 2019-10-25T12:30:10Z

Hello, I am working with Alison Mitchell at UZH this semester.

Adding a new muon ring fit algorithm, called Taubin fit.
The fit was referred to: https://www.zeuthen.desy.de/students/2018/Summerstudents2018/Reports/Pillera.pdf (eqn 9).
I will be working on adding a test file to show how it works.

ctapipe/image/muon/muon_ring_finder.py

maxnoe

This looks like it could be a single function, nothing here is really statefull.

codecov · 2019-10-25T12:50:11Z

Codecov Report

Merging #1154 into master will increase coverage by 0.09%.
The diff coverage is 90.51%.

@@            Coverage Diff             @@
##           master    #1154      +/-   ##
==========================================
+ Coverage   86.15%   86.24%   +0.09%     
==========================================
  Files         182      185       +3     
  Lines       11546    11600      +54     
==========================================
+ Hits         9947    10004      +57     
+ Misses       1599     1596       -3

Impacted Files	Coverage Δ
ctapipe/image/muon/tests/test_muon_fitting.py	`100% <ø> (ø)`	⬆️
ctapipe/image/muon/muon_ring_finder.py	`100% <100%> (ø)`	⬆️
ctapipe/io/containers.py	`100% <100%> (ø)`	⬆️
ctapipe/utils/tests/test_quantities.py	`100% <100%> (ø)`
ctapipe/utils/quantities.py	`100% <100%> (ø)`
ctapipe/image/muon/tests/test_muon_ring_finder.py	`100% <100%> (+4.91%)`	⬆️
ctapipe/image/muon/fitting.py	`54.63% <79.41%> (+16.97%)`	⬆️
ctapipe/image/muon/muon_reco_functions.py	`66.66% <83.33%> (ø)`	⬆️
ctapipe/image/muon/ring_fitter.py	`0% <0%> (-75%)`	⬇️
ctapipe/image/cleaning.py	`95.94% <0%> (-0.36%)`	⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a1ac92...6907f5b. Read the comment docs.

…o taubin_fit

dneise · 2019-10-25T15:33:16Z

Hello, thanks for the first review @maxnoe and @kosack.
You were totally right, there was no state, so we turned fit() into a classmethod. I was wondering if we should try to get the interface TaubinFitter.fit() as close to ChaudhuriKunduRingFitter.fit() as possible.

At the moment the problem is, that TaubinFitter.fit() would need some starting values, which basically only depend on the Camera type. So I assume they could be fed in via a config file... not sure though.

what do you think?

maxnoe · 2019-10-25T15:35:39Z

I'm confused. The kundu chauduri is a simple function:

ctapipe/ctapipe/image/muon/fitting.py

Line 23 in f34624a

def kundu_chaudhuri_circle_fit(x, y, weights):

maxnoe · 2019-10-25T15:37:21Z

I would also say that starting values are normally guessable. E.g. a weighted average for center x, y (or even just 0, 0) and the most common value (1.2 deg * focal length / fov) for the radius

dneise · 2019-10-25T15:43:34Z

Ah ... I think we were confused as well. You are looking at image/muon/fitting.py we were looking at image/muon/muon_ring_finder.py ...

Thanks for the hint, we'll have a closer look and come back later (after the weekend :-D)

kosack · 2019-10-28T09:50:56Z

There is a lot of refactoring to do in the Muon module, so i wouldn't worry too much about its current design - let's try to simplify it as much as possible.

I would suggest doing what we did in other modules: define everything as either a simple function, or as a class with __call__(..) set such that it acts like a function (if state and configuration needs to be stored between calls).

That way both the ChaudhuriKundu and Taubin fits are identical. The final design should support user configuration like the EventSource and CameraCalibrator classes, but if the algorithms are really simple, they don't need to be subclasses, we can just have a "MuonReconstructor" or whatever that calls the right function depending on the user's choice (e.g. an attribute called method)

AMWMitchell · 2019-10-28T10:27:00Z

Hi - does anyone know why "fitting.py" exists ? There is a version of the Chaudhuri-Kundu fit in there as a function, but it is never used.
We tried to mirror the implementation of the ChaudhuriKunduRingFitter in muon_ring_finder.py
--> This is the version that is actually used by muon_reco_function.
What changes are needed to the Taubin fit?
(Leaving future refactoring of the muon code aside for now.)

We should move the Taubin fit test from test_muon_fitting into test_muon_ring_finder.

maxnoe · 2019-10-28T10:29:20Z

I implemented the kundu chauduri function in fitting.py before the other classes were there.

I don't know why code review did not spot the duplication when the other things were merged.

AMWMitchell · 2019-10-28T10:39:35Z

Just checked the history and it seems both versions were created in October 2016.
The issue remains that the version in "fitting.py" (a bad name) was never used in the muon calibration chain.
Not sure if we've done a sanity check that the two implementations give the same results & open question on which to retain + how to structure the ring fitting (function / class).

For this pull request - what needs to change to the Taubin Fit ?

maxnoe · 2019-10-28T10:40:48Z

Like we said, just implement it as a simple function for now!

kosack · 2019-10-28T10:55:19Z

Just checked the history and it seems both versions were created in October 2016.
The issue remains that the version in "fitting.py" (a bad name) was never used in the muon calibration chain.

Can we just remove the non-used one? That sounds like a confusion that will cause problems soon!

kosack · 2019-10-28T10:56:04Z

(and sorry @momorning that this is becoming a discussion on re-factoring the muon module, but I think this PR can be a catalyst for that! - we can divide the work though)

dneise · 2019-10-28T10:59:20Z

Maybe we could quickly move the muon refactoring discussion into an issue...

AMWMitchell · 2019-10-28T11:05:42Z

Weirdly, I'm not sure that any of the functions in fitting.py are used. Many seem to be duplicated in e.g. muon_integrator.py (which is called by the muon chain) but structured differently.

Doesn't mean they are wrong, just we should work out what we want to keep.
A bigger discussion than this PR, but depending which version we prefer determines how to implement the TaubinFit - as a class like in muon_ring_finder or a function as in fitting ?

kosack · 2019-10-28T12:05:10Z

I think the question is just if it needs configuration or state between calls, if yes, it should be a class, if not, just a function (which we can wrap in a class later if we need to). For example, if those fits don't need any special state stored, you could define both as a function, but then just modify your MuonRingFinder class to call the right function depending on a user-configuration variable called "fit_method" or something (which is just a traits.CaselessStrEnum matching "taubin" or "chaudhury-kundu").

kosack · 2019-10-28T12:16:57Z

You should think about how a user would use it at the high level. I'd guess something like:

muonfit = MuonFitter(fit_method="taubin")
result = muonfit(x,y,weights,times)

and you want the user to be able to specify this in a config file, like:

'MuonFitter': { 
    'fit_method': 'taubin'
}

which could be achieved with something like:

def taubin(x, y, w, t):
   ... 

def chaudhury_kundu(x, y, w, t)
   ...


class MuonFitter(Component):
     fit_method = traits.CaselessStrEnum(
        ['taubin', 'chaudhury-kundu'], 
        default_value='taubin'
     ).tag(config=True)

    def __init__(self,  config=None, parent=None, **kwargs):
        super().__init__(config, parent, **kwargs)
        fit_map = {'taubin': taubin, 'chaudhury-kundu': chaudhury_kundu}
        self.__call__ = fit_map[self.fit_method]

(well you'd actually have to specify call better than that, since it needs the self argument, but you get the idea)

And presumably you'd make MuonFitter do all of the analysis, and the parameterization, etc, rather than just call the fit function, but the idea is the same

momorning · 2019-10-31T11:55:26Z

Code restructured, specify fit_method and tel_type when calling RingFitter
Example usage:
muonfit = MuonRingFitter(fit_method="taubin", tel_type = 'MST_MST_FlashCam')
muon_ring_parameters = muonfit.fit(x[mask],y[mask],img)

ctapipe/image/muon/muon_ring_finder.py

maxnoe

We are getting there! Let's restructure this more to separate the different implementations from the component and simplify things for testing and interpretation of the single methods.

One big problem is the telescope specfic starting values, that needs to be dealt with differently.

E.g. by using the newly introduced TelescopeParameter traitlet option. Or by just calculating decent values from focal length or what ever you need from the Instrument description.

ctapipe/image/muon/muon_ring_finder.py

kosack · 2019-10-31T15:29:15Z

It may be good to split thjis into 2 PRs:

define the taubin_fit function (as was started)
refactor the MuonReconstruction classes (the later discussion)

That will be more simple than doing it all at once, perhaps (but it's up to @momorning and @AMWMitchell )

ctapipe/utils/quantities.py

maxnoe

This is a large improvement!

dneise · 2019-12-04T14:42:38Z

I think we should not merge until some other core dev looked at this, maybe also @AMWMitchell wants to give a statement?

dneise · 2019-12-04T15:43:36Z

Hm tests are failing for the conda build .. .but I don't get why

dneise · 2019-12-04T15:44:26Z

Ah wait, it is also failing for the master .. so this is unrelated.

AMWMitchell · 2019-12-04T22:15:52Z

@dneise The restructuring looks good to me, but it is probably best reviewed (and accepted for merging) by @maxnoe and @kosack

dneise · 2019-12-09T09:09:44Z

Okay I re-ran the tests, they passed now. \o/ So maybe this can now be merged and then #1199 could be reviewed in more details, since the diff should be nice and small then? @kosack ?

…ing.

dneise · 2019-12-10T09:40:46Z

I modified the tests to be pytest.parametrized ... just saw this existed .. and think it applies here.
@maxnoe and @kosack please re-review if you have the time.

dneise · 2019-12-12T10:16:51Z

@kosack can we merge this one? The hough stuff is critical, I see ... but this one here should be fine, no?

add Taubin fit

f8e361e

kosack reviewed Oct 25, 2019

View reviewed changes

ctapipe/image/muon/muon_ring_finder.py Outdated Show resolved Hide resolved

maxnoe reviewed Oct 25, 2019

View reviewed changes

momorning and others added 4 commits October 25, 2019 16:26

taubin fit test

e6d6059

fixing typo

bd543ff

turn fit into classmethod, output MuonRingParameter

8a7ebb1

Merge branch 'taubin_fit' of https://github.com/momorning/ctapipe int…

eb2daf4

…o taubin_fit

remove unused variable, add a little docstring

2c2f278

momorning added 2 commits October 31, 2019 12:29

Class MuonRingFitter class with taubin and chaudhuri-kundu

ad4c800

removing test_taubin in test_muon_fitting

3a7be6f

maxnoe reviewed Oct 31, 2019

View reviewed changes

ctapipe/image/muon/muon_ring_finder.py Outdated Show resolved Hide resolved

maxnoe requested changes Oct 31, 2019

View reviewed changes

Dominik Neise added 2 commits December 3, 2019 13:47

rename

6015357

rename container fields

ff550e7

dneise force-pushed the taubin_fit branch from 6563d54 to ff550e7 Compare December 3, 2019 12:50

dneise previously approved these changes Dec 4, 2019

View reviewed changes

dneise requested a review from maxnoe December 4, 2019 08:55

maxnoe reviewed Dec 4, 2019

View reviewed changes

ctapipe/utils/quantities.py Outdated Show resolved Hide resolved

remove lying docs

646331b

dneise dismissed their stale review via 646331b December 4, 2019 14:37

dneise previously approved these changes Dec 4, 2019

View reviewed changes

further fix docs

7314f7c

dneise dismissed their stale review via 7314f7c December 4, 2019 14:39

maxnoe previously approved these changes Dec 4, 2019

View reviewed changes

dneise previously approved these changes Dec 4, 2019

View reviewed changes

dneise mentioned this pull request Dec 4, 2019

Add Hough fit #1199

Closed

dneise self-requested a review December 9, 2019 16:12

Dominik Neise added 2 commits December 10, 2019 10:35

use pytest parametrize for the test and make sure we do not test noth…

0090ad3

…ing.

fix typo

4dd76ab

dneise dismissed stale reviews from maxnoe and themself via 4dd76ab December 10, 2019 09:38

python/black

6907f5b

kosack approved these changes Dec 18, 2019

View reviewed changes

AMWMitchell approved these changes Dec 18, 2019

View reviewed changes

kosack merged commit 3171e67 into cta-observatory:master Dec 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add Taubin fit #1154

add Taubin fit #1154

momorning commented Oct 25, 2019

maxnoe left a comment

codecov bot commented Oct 25, 2019 •

edited

Loading

dneise commented Oct 25, 2019 •

edited

Loading

maxnoe commented Oct 25, 2019

maxnoe commented Oct 25, 2019

dneise commented Oct 25, 2019

kosack commented Oct 28, 2019

AMWMitchell commented Oct 28, 2019

maxnoe commented Oct 28, 2019

AMWMitchell commented Oct 28, 2019

maxnoe commented Oct 28, 2019

kosack commented Oct 28, 2019

kosack commented Oct 28, 2019

dneise commented Oct 28, 2019

AMWMitchell commented Oct 28, 2019

kosack commented Oct 28, 2019

kosack commented Oct 28, 2019 •

edited

Loading

momorning commented Oct 31, 2019 •

edited

Loading

maxnoe left a comment

kosack commented Oct 31, 2019

maxnoe left a comment

dneise commented Dec 4, 2019

dneise commented Dec 4, 2019

dneise commented Dec 4, 2019

AMWMitchell commented Dec 4, 2019

dneise commented Dec 9, 2019

dneise commented Dec 10, 2019

dneise commented Dec 12, 2019

add Taubin fit #1154

add Taubin fit #1154

Conversation

momorning commented Oct 25, 2019

maxnoe left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 25, 2019 • edited Loading

Codecov Report

dneise commented Oct 25, 2019 • edited Loading

maxnoe commented Oct 25, 2019

maxnoe commented Oct 25, 2019

dneise commented Oct 25, 2019

kosack commented Oct 28, 2019

AMWMitchell commented Oct 28, 2019

maxnoe commented Oct 28, 2019

AMWMitchell commented Oct 28, 2019

maxnoe commented Oct 28, 2019

kosack commented Oct 28, 2019

kosack commented Oct 28, 2019

dneise commented Oct 28, 2019

AMWMitchell commented Oct 28, 2019

kosack commented Oct 28, 2019

kosack commented Oct 28, 2019 • edited Loading

momorning commented Oct 31, 2019 • edited Loading

maxnoe left a comment

Choose a reason for hiding this comment

kosack commented Oct 31, 2019

maxnoe left a comment

Choose a reason for hiding this comment

dneise commented Dec 4, 2019

dneise commented Dec 4, 2019

dneise commented Dec 4, 2019

AMWMitchell commented Dec 4, 2019

dneise commented Dec 9, 2019

dneise commented Dec 10, 2019

dneise commented Dec 12, 2019

codecov bot commented Oct 25, 2019 •

edited

Loading

dneise commented Oct 25, 2019 •

edited

Loading

kosack commented Oct 28, 2019 •

edited

Loading

momorning commented Oct 31, 2019 •

edited

Loading