Move container definition into image module

[maxnoe]

This solves the circular import Problem @HealthyPear is facing in #1215 .

As image imported these containers from io, it imported the whole simetelevent source which imports the calibration stack which imports something from image.

[kosack]

with this cause (or solve) any circular imports?

[maxnoe]

@kosack solves, see new description.

[kosack]

Ok, though now reco will depend on image, but that may have already been the case.

[maxnoe]

This is the chain:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/maxnoe/Uni/CTA/ctapipe/ctapipe/image/__init__.py", line 1, in <module>
    from .hillas import *
  File "/home/maxnoe/Uni/CTA/ctapipe/ctapipe/image/hillas.py", line 11, in <module>
    from ..io.containers import HillasParametersContainer
  File "/home/maxnoe/Uni/CTA/ctapipe/ctapipe/io/__init__.py", line 8, in <module>
    from .simteleventsource import SimTelEventSource
  File "/home/maxnoe/Uni/CTA/ctapipe/ctapipe/io/simteleventsource.py", line 18, in <module>
    from ctapipe.calib.camera.gainselection import ThresholdGainSelector
  File "/home/maxnoe/Uni/CTA/ctapipe/ctapipe/calib/__init__.py", line 5, in <module>
    from .camera import *
  File "/home/maxnoe/Uni/CTA/ctapipe/ctapipe/calib/camera/__init__.py", line 6, in <module>
    from .calibrator import CameraCalibrator
  File "/home/maxnoe/Uni/CTA/ctapipe/ctapipe/calib/camera/calibrator.py", line 9, in <module>
    from ctapipe.image.extractor import NeighborPeakWindowSum
  File "/home/maxnoe/Uni/CTA/ctapipe/ctapipe/image/extractor.py", line 30, in <module>
    from ctapipe.image.timing_parameters import timing_parameters
  File "/home/maxnoe/Uni/CTA/ctapipe/ctapipe/image/timing_parameters.py", line 8, in <module>
    from .hillas import camera_to_shower_coordinates

[kosack]

It may be that we should have a separate module just for Container defs to avoid too many dependencies.

Probably is fine for a stop-gap solution though.

[maxnoe]

@kosack I like the fact that they are defined where they are used / emitted

[maxnoe]

Ah dam, I introduced another circular import.

[kosack]

I like the fact that they are defined where they are used / emitted

I do too, but often you don't need the algorithm to use the data, so if they are grouped, you entangle them

[kosack]

You can check with something like:

pydeps --show-cycles --max-bacon=2 --cluster --keep-target-cluster -T png ctapipe

see #1205

[maxnoe]

[maxnoe]

@kosack that does not really seem to work, as I back import the containers in io but it does not show a dependency from io to image
Or does it only show cycles?

[kosack]

Or does it only show cycles?

I think with those options, it only shows cycles. If you show everything it's huge.

But the cycles seem much improved.

remove "--show-cycles" to get the full thing, I believe (the bacon option limits the distance to show as well - it's the "Bacon number", as in Kevin Bacon)

[HealthyPear]

The errors in the log now seem to be different, they don't show anymore a circular import: there seems to be a problem with notebooks and the stub file from HillasParametersContainer

[maxnoe]

Ok, so I now moved all image related containers into the image submodule and removed them from io.

The only reason to remove them from io is that the docs create a warning, which is treated as error on travis. This is not good I think.

Also, does this create problems with the table reader? If the containers are not all in io/containers anymore?

Maybe better to just move the container definions into their own submodule? ctapipe.containers?

[codecov]

Codecov Report

Merging #1264 into master will increase coverage by 0.22%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1264      +/-   ##
==========================================
+ Coverage   86.78%   87.01%   +0.22%     
==========================================
  Files         192      194       +2     
  Lines       11967    12198     +231     
==========================================
+ Hits        10386    10614     +228     
- Misses       1581     1584       +3     

Impacted Files	Coverage Δ
ctapipe/io/containers.py	100.00% <ø> (ø)
ctapipe/image/__init__.py	100.00% <100.00%> (ø)
ctapipe/image/concentration.py	100.00% <100.00%> (ø)
ctapipe/image/containers.py	100.00% <100.00%> (ø)
ctapipe/image/hillas.py	100.00% <100.00%> (ø)
ctapipe/image/leakage.py	100.00% <100.00%> (ø)
ctapipe/image/muon/containers.py	100.00% <100.00%> (ø)
ctapipe/image/muon/muon_integrator.py	21.21% <100.00%> (+1.21%)	⬆️
ctapipe/image/muon/muon_ring_finder.py	100.00% <100.00%> (ø)
ctapipe/image/tests/test_hillas.py	100.00% <100.00%> (ø)
... and 65 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 0db9655...4aeeb30. Read the comment docs.

[HealthyPear]

Maybe better to just move the container definions into their own submodule? ctapipe.containers?

I would do this, it seems a cleaner approach