ChargeExtractor tests and simplification

[watsonjj]

This PR aims to improve the test for charge extractors, and begins the major refactoring described in #1026. Namely, the ChargeExtractors are significantly simplified with the intention that they more heavily use a composition approach instead of inheritance in the future.

[codecov]

Codecov Report

Merging #1023 into master will decrease coverage by 0.01%.
The diff coverage is 97.65%.

@@            Coverage Diff             @@
##           master    #1023      +/-   ##
==========================================
- Coverage    83.6%   83.59%   -0.02%     
==========================================
  Files         189      189              
  Lines       10726    10647      -79     
==========================================
- Hits         8968     8900      -68     
+ Misses       1758     1747      -11

Impacted Files	Coverage Δ
ctapipe/tools/display_dl1.py	90% <ø> (ø)	⬆️
ctapipe/tools/display_integrator.py	96.79% <ø> (ø)	⬆️
ctapipe/tools/extract_charge_resolution.py	74.64% <ø> (ø)	⬆️
ctapipe/tools/bokeh/file_viewer.py	72.83% <0%> (-0.29%)	⬇️
ctapipe/image/waveform_cleaning.py	98.36% <100%> (ø)	⬆️
ctapipe/image/charge_extractors.py	94.11% <100%> (+4.67%)	⬆️
ctapipe/image/tests/test_charge_extraction.py	100% <100%> (ø)	⬆️

---

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 cdc0c45...5baf334. Read the comment docs.

[dneise]

Since the title is "Major refactoring" I am already frightened :-D

So in case you wanna make review easier, here just some thoughts:

If one only modifies tests or adds tests, but not the code base, this is quite simple to review I think.
If one does not touch the tests, but refactors the code base, this is quite easy to review as well.
(I did not follow my own advice in the past, I know)

So if you happen to be done already (or partly done) with refactoring and adding some tests, we could already merge it.

[watsonjj]

I get where you are coming from. I just thought this process would be a bit messy if split into many pull requests. Not sure what is the best way in Github to handle a major pull request with many sub-pull-requests. I will try to split it into stages.

[maxnoe]

If the single diffs are easy to review, that's maybe enough.

[dneise]

Cool, thx. I can already say, that I like the current version better than the master. The master tested only the bare minimum, i.e. if the extractors could be called at all ... but it never tested the results.

[dneise]

If the single diffs are easy to review, that's maybe enough.

yeah ... you are right. I should start reviewing that way and not only look at the end result.

[watsonjj]

Okay, this is the first PR for the refactoring of the ChargeExtractors. I have now created the issue #1026 to organise the large amount of planned changes.

I began this PR by creating some tests for the return values of the charges, to make sure I maintain results in my changes. I have then simplified the implementations of the ChargeExtractor.

[watsonjj]

This PR should not break API either

[watsonjj]

Python 3.7 failing in travis due to pandoc version (I think)

[maxnoe]

Travis passed...

[watsonjj]

Okay... I will use the X = (d * m).sum(axis=-1) form as it is the most readable, while providing some (albeit premature) optimisation.

[maxnoe]

Why multiply and not index? I find it much more readable to use the index form.

[watsonjj]

Why multiply and not index? I find it much more readable to use the index form.

Need to keep the shape/dimensions for the sum. Indexing flattens the array. Could reshape back, but theres the potential that one could have different number of samples in integration window for different pixels.

[kosack]

Should this be tagged work in progress? or is it basically done?

[watsonjj]

This part is done

[watsonjj]

Ah, I have realised a problem with this approach - configuring the window_width and window_shift traitlets cannot be easily done anymore.

Before, one could specifiy the following in a Tool:

    aliases = Dict(
        dict(
            window_width='WindowIntegrator.window_width',
            window_shift='WindowIntegrator.window_shift',
        )
    )

But now, because the different width and shift traitlets are attached to each ChargeExtractor independently, one would have to include the traitlet for each ChargeExtractor.

The only way I see to fix this is keep some inheritance of a common WindowIntegrator class, which defines these traitlets.

[kosack]

Just a comment, I think it is ok to use einsum if it's a real speed gain, but only if we include a comment explaining exactly what it's doing (e.g. write it in tensor notation so we can understand the expectation without reading the manual for einsum, which is always a bit confusing due to the syntax).

[kosack]

Of course, from what I've learned at PyGamma, you can also get some of the benefits of einsum by using Numba on a numpy function, keeping the readability (but then you have to ensure the function has simple ndarray inputs and outputs)

[watsonjj]

using Numba

I intend to do an optimisation study towards the end of #1026, which will include using numba.

[watsonjj]

I'm not sure what is up with codacy, but this PR is ready for reviews and merge

[watsonjj]

I have adopted the approach mentioned in #1029, which suggests to more sparingly use the aliases feature. This therefore allows me to remove the abstract classes once again, returning to the simple Component-inheritence (only inherit from ChargeExtractor) I had intended for this PR.

[dneise]

I like this ... I have a few questions and remarks, but the overall structure looks very nice now.

[dneise]

I have to run, so I approve now not waiting for the tests to pass. If they do not pass .... I obviously do not really approve :-D

[watsonjj]

@kosack Could you restore your approval for this PR? Or @thomasgas?