2-Pass waveform integration à-la-CTA/MARS #1215

HealthyPear · 2020-01-28T14:08:19Z

Fixes #602

Prerequisites:

Merge PR Cleaning for biggest cluster #1131 - continuous integration will fail until this is done.

Description:

ctapipe-based version of the calibration method used in CTA/MARS and described also here.
triggered from development of protopipe and comparison against CTA/MARS (in particular DL1, see
Comparison with CTA-MARS: Calibration protopipe#31 and Comparison with CTA-MARS: Image cleaning & parametrization protopipe#37 for more information)

Caveats:

There is at least one cut inside this image extractor, between the 1st and 2nd pass (n_pixels > 3); in ctapipe we shouldn't do this, aka any image extractor should always return charge and pulse times; what I am testing in protopipe and I propose to do also here, is to add a 3rd return argument (at this point to any image extractor) named status, which acts like a "quality label", so later the user can select events on the basis of image extraction quality for more complex image extractors that allow internal selection (like this one),
This triggers another problem: in the current protopipe/CTA-MARS what is compared and enters in the benchmark of the charge resolution is the 1st pass, whereas what enters in the final image cleaning is the 2nd pass; this image extractor should then export both passes, but right now in ctapipe only 1 charge and 1 pulse time per image is exported by the call to the calibration method
To be as much as close to CTA/MARS as possible, the ctapipe.image.timing_parameters should be modified in order to use a robust fitting algorithm (like scipy.stats.siegelslopes), though I couldn't find an easily importable solution which treated also the fit errors.

…; PEP8-compliant formatting.

kosack · 2020-02-19T16:41:08Z

ctapipe/image/extractor.py

+        Array containing the sums for each of the kernel positions.
+        Shape: (n_samples - window_width - 1)
+
+    Dr. Michele Peresano, 2019


I'd suggest removing your name here, and instead adding it to the CODEOWERS file at the top level (we decided against putting names in the comments). You also should also update AUTHORS and .mailmap with your information

However, if there is a reference for the MARS analysis, it would be nice to add it also to docs/references.rst and add a citation link. Note that it should not be in the Returns section (sphinx won't like that), but in either the description or See Also section (see https://numpydoc.readthedocs.io/)

I'll try to see what could be the right citation

If I understood it correctly is this the two-pass integration method described in section 4 of https://ui.adsabs.harvard.edu/abs/2006APh....25..391H/abstract ?

If this is correct, then it maybe shouldn't be called CTA/mars analysis :-)

Hi Gernot, I apologize for the late reply.

I don't know if the version of the algorithm I coded here comes directly from the reference that you provide (in any case, from what you say it seems a necessary reference to be added to the documentation so I will do it).
I just tried with as much fidelity as possible to translate in ctapipe-language the version that is used inside CTA/MARS.
That is the reason for the name of the PR (if this is what you refer to).
I also explicited in the class documentation that "[...] This is in particular the CTA-MARS version.".

For me there is no problem in changing names, perhaps we can ask @moralejo for a confirmation on this.

Hi, the Eventdisplay reference is indeed to be included: I implemented this in MARS for the CTA analysis (it is not used in MAGIC) to get closer to Eventdisplay. Still, in the spirit of having fully independent analysis codes, I just took the idea, "make a time fit with significant pixels, then do a second round of pulse integration on the rest of the pixels using the times 'predicted' by the fit". The implementation was done in a completely independent manner, and it is bound to differ in the details. I do not think it makes much sense to copy every detail of the Mars version for protopipe - as you @HealthyPear realized, some things (like ROOT's "robust" fit details) would take quite some effort and are probably not essential.

Hi, the Eventdisplay reference is indeed to be included: I implemented this in MARS for the CTA analysis (it is not used in MAGIC) to get closer to Eventdisplay. Still, in the spirit of having fully independent analysis codes, I just took the idea, "make a time fit with significant pixels, then do a second round of pulse integration on the rest of the pixels using the times 'predicted' by the fit". The implementation was done in a completely independent manner, and it is bound to differ in the details.

I will then add the Eventdisplay one; do you have any suggestion for the reference on the CTA/MARS side? Meanwhile, I took the liberty to add the URL of the wiki page as a citation.

I'd suggest removing your name here,

Done

and instead adding it to the CODEOWERS file at the top level (we decided against putting names in the comments).

Done - I hope is the correct syntax

You also should also update AUTHORS and .mailmap with your information

I have already done this sometime ago, do I have to change or update something?

I will then add the Eventdisplay one; do you have any suggestion for the reference on the CTA/MARS side? Meanwhile, I took the liberty to add the URL of the wiki page as a citation.

Unfortunately we have no public reference for that, since as I said it is not a method we use in the MAGIC analysis. Wiki page is ok for internal reference.

possibly the ICRC proceeding by Maxim Shayduk - I think that's the origin of the method.

In particular: - array indexes were wrong by 1 - initial window sliding doesn't touch waveform extremes - in 2nd pass windth and shift of out-of-readout predicted times were not modified

…object.

ctapipe/image/extractor.py

maxnoe · 2020-04-07T11:56:12Z

ctapipe/image/extractor.py

 from ctapipe.core import Component
 from numba import njit, prange, guvectorize, float64, float32, int64

+from ctapipe.image.cleaning import number_of_islands, largest_island, tailcuts_clean


use relative imports here.

maxnoe · 2020-04-08T09:58:50Z

@HealthyPear #1267 is merged, if you merge or rebase vs master, this should work now.

ctapipe/image/extractor.py

HealthyPear · 2020-04-10T14:35:24Z

ok lots of discussion here! Thanks for you input

ctapipe/image/tests/test_extractor.py

watsonjj · 2020-05-04T16:27:09Z

There are some codacy complaints you should fix

kosack

I think if you fix a few Codacy complaints + implement @watsonjj's suggestion to split the __call__() function into two sub-methods, it will be ready.

For call, something like:

def __call__(self, waveforms, telid, selected_gain_channel):
     charge1, pulse_time1 = self.apply_pass_1(waveforms, telid, selected_gain_channel)
     self.apply_pass_2(waveforms, telid, selected_gain_channel, charge1, pulse_time1)

That way, you can just call apply_pass_1 for doing the benchmarks.

HealthyPear · 2020-05-05T14:14:50Z

I think if you fix a few Codacy complaints + implement @watsonjj's suggestion to split the __call__() function into two sub-methods, it will be ready.

For call, something like:
def __call__(self, waveforms, telid, selected_gain_channel):
     charge1, pulse_time1 = self.apply_pass_1(waveforms, telid, selected_gain_channel)
     self.apply_pass_2(waveforms, telid, selected_gain_channel, charge1, pulse_time1)
That way, you can just call apply_pass_1 for doing the benchmarks.

Yes, it looks like a neat solution....

Also in between the two, there is the preliminary image cleaning from which the actual return values are obtained by checking the result.

- call to the class has been splitted, - bugs have been found and fixed following bugfix in unit-test - given the fixed simulated waveform, the relative tolerance for this image extractor can be now pushed down to 7%

Added new TwoPassWindowSum ImageExtractor instance with its unit-test…

6abcb5a

…; PEP8-compliant formatting.

HealthyPear requested a review from watsonjj as a code owner January 28, 2020 14:08

HealthyPear mentioned this pull request Jan 28, 2020

Comparison with CTA-MARS: Calibration cta-observatory/protopipe#31

Closed

4 tasks

HealthyPear added algorithm calibration enhancement labels Feb 6, 2020

kosack reviewed Feb 19, 2020

View reviewed changes

Michele Peresano added 3 commits February 21, 2020 13:41

Updated slide_windows with better docs and variable names.

1e8ee5a

Corrected and updated TwoPassWindowSum.

ed8e89e

In particular: - array indexes were wrong by 1 - initial window sliding doesn't touch waveform extremes - in 2nd pass windth and shift of out-of-readout predicted times were not modified

Fixed error for which array images were referring always to the same …

0795d9b

…object.

kosack requested changes Feb 21, 2020

View reviewed changes

ctapipe/image/extractor.py Show resolved Hide resolved

Michele Peresano added 3 commits February 21, 2020 16:19

Changed core thresholds as TelescopeParameter.

67f804f

Added references to class documentation.

219b3b7

Updated CODEOWNERS file.

4b93a59

HealthyPear mentioned this pull request Feb 26, 2020

Double-pass image extractor cta-observatory/protopipe#48

Merged

kosack previously approved these changes Mar 17, 2020

View reviewed changes

Michele Peresano added 3 commits March 25, 2020 16:20

Merge branch 'master' into feature/TwoPassWindowSum_ImageExtractor

37fc9a8

Merge branch 'master' into feature/TwoPassWindowSum_ImageExtractor

04efa88

Merge branch 'master' into feature/TwoPassWindowSum_ImageExtractor

85c1603

HealthyPear mentioned this pull request Apr 6, 2020

Add other cameras to toy waveforms generator #1255

Closed

HealthyPear dismissed kosack’s stale review via 679148f April 7, 2020 10:10

Michele Peresano added 2 commits April 7, 2020 12:10

Fixed merge conflicts and updated code ( class definition + unit-test).

679148f

Fixed documentation references.

eaaaf54

maxnoe reviewed Apr 7, 2020

View reviewed changes

maxnoe mentioned this pull request Apr 7, 2020

Move container definition into image module #1264

Closed

watsonjj added this to the v0.8.0 milestone Apr 8, 2020

Michele Peresano and others added 3 commits April 8, 2020 14:48

Merge branch 'master' into feature/TwoPassWindowSum_ImageExtractor

d82a42e

Merge branch 'master' into feature/TwoPassWindowSum_ImageExtractor

309cb29

Fix docstring formatting

ddd53b3

watsonjj requested changes Apr 10, 2020

View reviewed changes

ctapipe/image/extractor.py Show resolved Hide resolved

This was referenced Apr 13, 2020

Implement 2-pass trace integration #602

Closed

use a robust fitting in timing_parameters #1275

Closed

Michele Peresano added 6 commits May 4, 2020 13:12

Revert subarray as a specific argument in ImageExtractor initialization.

5c33d02

use relative imports

88cf166

Merge branch 'master' into feature/TwoPassWindowSum_ImageExtractor

d314201

Added changes required to run the test as before.

44919b7

Added parametrizable toymodel

8af5cb6

Updated test function

687f30e

watsonjj requested changes May 4, 2020

View reviewed changes

ctapipe/image/tests/test_extractor.py Show resolved Hide resolved

Call get_test_toymodel in toymodel to remove duplicated code.

d4e1490

kosack requested changes May 5, 2020

View reviewed changes

HealthyPear added 9 commits May 6, 2020 14:38

Merge branch 'master' into feature/TwoPassWindowSum_ImageExtractor

d4be780

Make _calculate_correction a private method for each image extractor.

f44f934

Hide unused variable for transverse COG coordinate.

1cc8828

Fix error in test (it was always running only for 1 to 10 phe!)

1a648a5

Multiple changes, see description.

058b9f2

- call to the class has been splitted, - bugs have been found and fixed following bugfix in unit-test - given the fixed simulated waveform, the relative tolerance for this image extractor can be now pushed down to 7%

Few formatting changes via black.

399c411

Merge branch 'master' into feature/TwoPassWindowSum_ImageExtractor

0e03efa

Spurious lines from merge conflict removed.

6f55ce6

Removed unnecessary 'else' clauses.

4e20ffe

HealthyPear mentioned this pull request May 7, 2020

Add information about quality of calibration #1317

Open

watsonjj approved these changes May 7, 2020

View reviewed changes

watsonjj requested a review from kosack May 11, 2020 09:04

kosack approved these changes May 11, 2020

View reviewed changes

kosack merged commit d8a8718 into cta-observatory:master May 11, 2020

HealthyPear deleted the feature/TwoPassWindowSum_ImageExtractor branch April 9, 2021 09:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2-Pass waveform integration à-la-CTA/MARS #1215

2-Pass waveform integration à-la-CTA/MARS #1215

HealthyPear commented Jan 28, 2020 •

edited by kosack

Loading

kosack Feb 19, 2020

kosack Feb 19, 2020

HealthyPear Feb 21, 2020

GernotMaier Feb 21, 2020

HealthyPear Feb 24, 2020

moralejo Feb 25, 2020

HealthyPear Feb 25, 2020 •

edited

Loading

HealthyPear Feb 25, 2020

moralejo Feb 26, 2020

kosack Feb 26, 2020

maxnoe Apr 7, 2020

maxnoe commented Apr 8, 2020

HealthyPear commented Apr 10, 2020

watsonjj commented May 4, 2020

kosack left a comment

HealthyPear commented May 5, 2020

2-Pass waveform integration à-la-CTA/MARS #1215

2-Pass waveform integration à-la-CTA/MARS #1215

Conversation

HealthyPear commented Jan 28, 2020 • edited by kosack Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HealthyPear Feb 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxnoe commented Apr 8, 2020

HealthyPear commented Apr 10, 2020

watsonjj commented May 4, 2020

kosack left a comment

Choose a reason for hiding this comment

HealthyPear commented May 5, 2020

HealthyPear commented Jan 28, 2020 •

edited by kosack

Loading

HealthyPear Feb 25, 2020 •

edited

Loading