Drop support for CameraFrame in ImageProcessor #2141

StFroese · 2022-11-28T09:01:22Z

Initially I wanted to remove the option to use the CameraFrame for calculating the Hillas parameters, but I had to change a lot of stuff and ended up removing the CameraFrame completely from the ImageProcessor.

fixes #2061

codecov · 2022-11-28T09:13:25Z

Codecov Report

Base: 92.75% // Head: 92.61% // Decreases project coverage by -0.14% ⚠️

Coverage data is based on head (0fcf8f9) compared to base (0e4ffea).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 0fcf8f9 differs from pull request most recent head ce99544. Consider uploading reports for the commit ce99544 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2141      +/-   ##
==========================================
- Coverage   92.75%   92.61%   -0.14%     
==========================================
  Files         216      216              
  Lines       18050    17888     -162     
==========================================
- Hits        16742    16567     -175     
- Misses       1308     1321      +13

Impacted Files	Coverage Δ
ctapipe/image/tests/test_image_processor.py	`100.00% <ø> (ø)`
ctapipe/io/tests/test_hdf5eventsource.py	`97.63% <ø> (-0.22%)`	⬇️
ctapipe/reco/tests/test_HillasReconstructor.py	`100.00% <ø> (+0.74%)`	⬆️
ctapipe/tools/process.py	`86.66% <ø> (ø)`
ctapipe/tools/tests/test_process.py	`100.00% <ø> (ø)`
ctapipe/image/concentration.py	`100.00% <100.00%> (ø)`
ctapipe/image/extractor.py	`83.45% <100.00%> (+0.06%)`	⬆️
ctapipe/image/hillas.py	`98.46% <100.00%> (+1.44%)`	⬆️
ctapipe/image/image_processor.py	`100.00% <100.00%> (ø)`
ctapipe/image/muon/tests/test_ring_fitter.py	`100.00% <100.00%> (ø)`
... and 15 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

kosack · 2022-11-28T10:11:08Z

Thanks this will clean up a lot of redundant code! It's clear a lot of pieces need to be touched, so that is ok. The only consequence is that it means pointing corrections and the focal length are "baked in" to the DL1 parameters (assuming they are applied in the CameraFrame → TelescopeFrame transition), but I think that is a small price to pay for simplicity, and if the need arises we can add a way to correct them.

kosack · 2022-11-28T10:12:44Z

You'll need to update some of the tutorials as well, as some were written before there was a TelescopeFrame I'm afraid!

kosack

Since so much had to change, it may be a good idea for somebody to run an analysis (both regular reconstruction as well as muon analysis) on a large number of events and compare the results to the old version before we merge this. Just in case it introduces some unexpected typos.

maxnoe · 2022-12-07T13:46:01Z

There is a lot of code in ctapipe/reco/hillas_reconstructor and ctapipe/reco/hillas_intersection dealing with parameters in camera frame, these should also be removed.

ctapipe/reco/hillas_intersection.py

review-notebook-app · 2023-01-09T15:49:20Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

StFroese · 2023-01-10T11:01:09Z

I checked the dl1 parameters for this branch and master for regular and for the muon analysis. Works fine.
Also updated the jupyter notebooks in the docs

LukasNickel

Looks almost done to me. Some comments:

If we do not support loading camera frame files, we need to put that in the changelog. I am fine with it, it is bound to happen at some point
Strip notebooks
Fix merge conflicts

ctapipe/io/hdf5eventsource.py

ctapipe/reco/impact.py

ctapipe/image/extractor.py

docs/examples/camera_display.ipynb

docs/changes/2141.api.rst

kosack · 2023-01-16T10:58:18Z

ctapipe/image/extractor.py

@@ -1051,7 +1052,9 @@ def _apply_second_pass(
        # Apply correction to 1st pass charges
        charge_1stpass = charge_1stpass_uncorrected * correction[selected_gain_channel]

-        camera_geometry = self.subarray.tel[tel_id].camera.geometry
+        camera_geometry = self.subarray.tel[tel_id].camera.geometry.transform_to(
+            TelescopeFrame()


this might need some thought - we shouldn't be transforming the geometry for each event inside a calibration/image-extraction algorithm like this as that adds quite a bit of overhead. This algorithm does not care about the frame of the camera geometry or pointing corrections, so it should not require a transform.

Since we do eventually need to transform for the ImageProcessor, it may be better to do the transform outside of these algorithms, perhaps in the CameraCalibrator (which eventually should also apply pointing correcions), so that a single TelescopeFrame is available to all algorithms that need it. That would avoid re-doing the transform many times.

Yes, this should be done inside the ProcessorTool in setup after writing out the subarray description but before setting up telescope components. The telescope components should just get a subarray with cameras in CameraFrame.

Maybe add transform_camera_geometries to SubarrayDescription returning a new Subarray with transformed geometries

Is creating a whole new SubarrayDescription each time efficient? This was an issue I think we discussed a while back (need to find where): during analysis, the TelescopeFrame needs to be updated once per event for observations, and could be simplified to only once per run for simulations in fixed Alt/Az mode.

The TelescopeFrame requires:

the latest pointing position, which changes in time in ICRS tracking mode (the default for most observations)

pre-computed pointing corrections which could include not just a shift in RA/Dec, but also a rotation (this is not currently implemented, but will need to be done eventually)

Therefore where to store these frames is a question. Normally the SubarrayDescription is a static thing, but it could go there, but re-generating a whole SubarrayDescription might be wasteful if we could just update the info. Note that we don't want to re-generate neighbor pixel info in CameraGeometry for each event, as that does not change.

Is creating a whole new SubarrayDescription each time efficient?

Not each time. Once in the tool in setup.

Ah, sorry, forgot about pointing corrections.

But it stands: we could also do the pointing corrections in the transformation from / to TelescopeFrame to higher frames. That would actually make the most sense to me, since pointing direction is not important for the CameraFrame <-> TelescopeFrame transformation.

To me that means we should transform the whole subarray once to TelescopeFrame and apply corrections in the transformation AltAz <-> TelescopeFrame, we don't need to change telescope frame for this, as the attributes needed for the transformation can be specified on the SkyCoord as seen here:

ctapipe/ctapipe/visualization/mpl_camera.py

Lines 434 to 439 in 8a7a367

coord : `~astropy.coordinates.SkyCoord`

The coordinate to plot. Must be able to be transformed into

the frame of the camera geometry of this display.

Most of the time, this means you need to add the telescope

pointing as a coordinate attribute like this:

``SkyCoord(..., telescope_pointing=pointing)``

Remember there are in principle rotation corrections and maybe others, so you have to apply pointing corrections before Hillas parameterization (doing it while going to Alt/Az wouldn't work), as you need to have correctly shifted/rotated parameters before you do shower geometry reconstruction. But that can be solved when the ShowerProcessor is moved to TelescopeFrame later (right now e.g. HillasReconstructor supports both frames, but that's ugly).

@maxnoe @kosack please have a look now if this solves the issue

maxnoe · 2023-01-31T10:22:13Z

ctapipe/image/tests/test_hillas.py

-    y=0.3 * u.m,
-    width=0.05 * u.m,
-    length=0.15 * u.m,
+    x=0.2 * u.deg,


Did you check that these images make sense when just changing meters to degrees?

I checked for the notebooks, I can recheck for all other files

maxnoe · 2023-01-31T10:24:30Z

ctapipe/image/toymodel.py

@@ -80,8 +87,8 @@ def obtain_time_image(x, y, centroid_x, centroid_y, psi, time_gradient, time_int

    """
    longitudinal, _ = camera_to_shower_coordinates(x, y, centroid_x, centroid_y, psi)
-    longitudinal_m = longitudinal.to_value(u.m)
-    time_gradient_ns_m = time_gradient.to_value(u.ns / u.m)
+    longitudinal_m = longitudinal.to_value(u.deg)


variable names still contain the old units

maxnoe · 2023-01-31T10:25:23Z

docs/changes/2134.bug.rst

@@ -0,0 +1 @@
+Fix for Hillas lines in ``ArrayDisplay`` being wrong in the new ``EastingNorthingFrame``.


Is that part of this PR?

I think something went wrong while rebasing. It's not part of the PR, I thinks it's an old one

maxnoe · 2023-01-31T10:25:37Z

docs/changes/2155.bug.rst

@@ -0,0 +1 @@
+Replace usage of ``$HOME`` with ``Path.home()`` for cross-platform compatibility.


Not part of this PR

maxnoe · 2023-11-17T11:49:36Z

superseded by #2396

StFroese added the refactoring label Nov 28, 2022

StFroese requested review from HealthyPear and watsonjj as code owners November 28, 2022 09:01

StFroese linked an issue Nov 28, 2022 that may be closed by this pull request

remove support for calculating Hillas parameters in CameraFrame #2061

Open

StFroese requested review from maxnoe and kosack November 28, 2022 09:02

kosack reviewed Nov 28, 2022

View reviewed changes

kosack changed the title ~~Drop support for CamerFrame in ImageProcessor~~ Drop support for CameraFrame in ImageProcessor Nov 28, 2022

kosack added the API change label Nov 28, 2022

StFroese mentioned this pull request Nov 30, 2022

Update Docs and Tutorials #2143

Open

33 tasks

maxnoe reviewed Dec 7, 2022

View reviewed changes

ctapipe/reco/hillas_intersection.py Outdated Show resolved Hide resolved

StFroese force-pushed the remove_hillas_cameraframe branch from af5140c to b4c3235 Compare January 9, 2023 16:07

StFroese requested review from maxnoe, kosack, nbiederbeck and LukasNickel January 10, 2023 11:01

LukasNickel requested changes Jan 10, 2023

View reviewed changes

StFroese force-pushed the remove_hillas_cameraframe branch from b4c3235 to 1e3f59b Compare January 11, 2023 11:49

StFroese requested a review from LukasNickel January 12, 2023 14:39

maxnoe added this to the v0.18.0 milestone Jan 13, 2023

StFroese force-pushed the remove_hillas_cameraframe branch from 469b958 to dd1b6d1 Compare January 13, 2023 12:57

StFroese added 3 commits January 13, 2023 13:57

remove CameraFrame from hillas

d0b8083

remove CameraHillasParametersContainer option

8034cee

extraction in TelescopeFrame

a004399

StFroese added 5 commits January 13, 2023 14:00

remove CameraHillasParametersContainer

37fe0e7

speedup using frame instead of SkyCoord

2bc2b3e

Merge branch 'master' into remove_hillas_cameraframe

9af1cf7

pixel_positions for TelescopeFrame

2f3c6de

use TelescopeFrame in doc notebooks

712e436

StFroese force-pushed the remove_hillas_cameraframe branch from dd1b6d1 to 467be42 Compare January 13, 2023 13:15

StFroese added 2 commits January 13, 2023 14:47

Merge branch 'master' into remove_hillas_cameraframe

84ceeb7

fix CameraDisplay test

de32018

StFroese force-pushed the remove_hillas_cameraframe branch from 467be42 to de32018 Compare January 13, 2023 13:47

kosack requested changes Jan 16, 2023

View reviewed changes

maxnoe modified the milestones: v0.18.0, v0.19.0 Jan 24, 2023

StFroese added 3 commits January 30, 2023 15:39

move geometry transformation to setup

72c33b2

subarray transform to TelescopeFrame

3128997

Merge branch 'master' into remove_hillas_cameraframe

ce99544

StFroese requested a review from kosack January 31, 2023 10:05

maxnoe reviewed Jan 31, 2023

View reviewed changes

kosack mentioned this pull request Mar 2, 2023

Remove or move examples to documentation #2266

Merged

maxnoe mentioned this pull request Jun 5, 2023

First step of implementing CEP-002: Change container structure #2204

Open

This was referenced Jun 13, 2023

CameraDisplay does not support hillas parameters in TelescopeFrame for overlay_moments. #2346

Closed

Toymodel does not support geometry / model in telescope frame #2348

Closed

kosack modified the milestones: v0.20.0, v0.21.0 Sep 8, 2023

StFroese mentioned this pull request Sep 15, 2023

Drop camera frame #2396

Open

maxnoe closed this Nov 17, 2023

maxnoe removed this from the v0.21.0 milestone Mar 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop support for CameraFrame in ImageProcessor #2141

Drop support for CameraFrame in ImageProcessor #2141

StFroese commented Nov 28, 2022 •

edited by kosack

Loading

codecov bot commented Nov 28, 2022 •

edited

Loading

kosack commented Nov 28, 2022

kosack commented Nov 28, 2022

kosack left a comment

maxnoe commented Dec 7, 2022

review-notebook-app bot commented Jan 9, 2023

StFroese commented Jan 10, 2023

LukasNickel left a comment

kosack Jan 16, 2023

maxnoe Jan 16, 2023

maxnoe Jan 16, 2023

kosack Jan 16, 2023 •

edited

Loading

maxnoe Jan 16, 2023

maxnoe Jan 16, 2023

kosack Jan 30, 2023

StFroese Jan 31, 2023

maxnoe Jan 31, 2023

StFroese Jan 31, 2023

maxnoe Jan 31, 2023

maxnoe Jan 31, 2023

StFroese Jan 31, 2023

maxnoe Jan 31, 2023

maxnoe commented Nov 17, 2023

	coord : `~astropy.coordinates.SkyCoord`
	The coordinate to plot. Must be able to be transformed into
	the frame of the camera geometry of this display.
	Most of the time, this means you need to add the telescope
	pointing as a coordinate attribute like this:
	``SkyCoord(..., telescope_pointing=pointing)``

		@@ -0,0 +1 @@
		Fix for Hillas lines in ``ArrayDisplay`` being wrong in the new ``EastingNorthingFrame``.

		@@ -0,0 +1 @@
		Replace usage of ``$HOME`` with ``Path.home()`` for cross-platform compatibility.

Drop support for CameraFrame in ImageProcessor #2141

Drop support for CameraFrame in ImageProcessor #2141

Conversation

StFroese commented Nov 28, 2022 • edited by kosack Loading

codecov bot commented Nov 28, 2022 • edited Loading

Codecov Report

kosack commented Nov 28, 2022

kosack commented Nov 28, 2022

kosack left a comment

Choose a reason for hiding this comment

maxnoe commented Dec 7, 2022

review-notebook-app bot commented Jan 9, 2023

StFroese commented Jan 10, 2023

LukasNickel left a 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

kosack Jan 16, 2023 • 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

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

maxnoe commented Nov 17, 2023

StFroese commented Nov 28, 2022 •

edited by kosack

Loading

codecov bot commented Nov 28, 2022 •

edited

Loading

kosack Jan 16, 2023 •

edited

Loading