Generalize image converter for square pixels #1728

LukasNickel · 2021-05-26T18:17:15Z

This add new converter functions to go from 1d to 2d images and backwards with square pixels (which is the much easier case obviously). It is basically the code from #1139 and added to be able to apply a gaussian filter in #1723 (at least for square pixels).

I see some issues with this structure of converters and I am also not really happy with the API, but for now I wanted to make minimal changes to the code. Maybe this is something that could be attached to the camera geometry in the future?
Also this is pretty trivial to vectorize, but I am not sure about the existing code for hexagonal pixels, so I skipped on that as well (and I intend to use it in an event loop for now).

Bonus:
The existing unit test was not really doing much, because it compared the pixel shape to a string and thus skipped most geometries. Thats fixed as well.

pixels - Removes the old astri and chec code as this is now handled in the general case

- Test now compares the original image to the image after tranforming back to 1d - Fixes loop, removes return: Previously most geometries were actually untested, because the pixel shape was compared to a string instead of the enum. This printed a warning about unsupported camera geometries but that was easily missed.

codecov · 2021-05-26T18:19:51Z

Codecov Report

Merging #1728 (d499d99) into master (6d0051e) will increase coverage by 0.69%.
The diff coverage is 85.48%.

@@            Coverage Diff             @@
##           master    #1728      +/-   ##
==========================================
+ Coverage   91.47%   92.16%   +0.69%     
==========================================
  Files         184      182       -2     
  Lines       14341    14890     +549     
==========================================
+ Hits        13119    13724     +605     
+ Misses       1222     1166      -56

Impacted Files	Coverage Δ
ctapipe/image/__init__.py	`100.00% <ø> (ø)`
ctapipe/instrument/camera/image_conversion.py	`70.00% <70.00%> (ø)`
ctapipe/instrument/camera/geometry.py	`91.43% <100.00%> (+1.29%)`	⬆️
...e/instrument/camera/tests/test_image_conversion.py	`100.00% <100.00%> (ø)`
ctapipe/tools/display_dl1.py	`83.67% <0.00%> (-6.81%)`	⬇️
ctapipe/utils/download.py	`82.29% <0.00%> (-4.48%)`	⬇️
ctapipe/tools/stage1.py	`94.44% <0.00%> (-3.48%)`	⬇️
ctapipe/conftest.py	`97.60% <0.00%> (-2.41%)`	⬇️
ctapipe/tools/tests/test_merge.py	`100.00% <0.00%> (ø)`
ctapipe/utils/filelock.py	`76.19% <0.00%> (ø)`
... and 11 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 6d0051e...d499d99. Read the comment docs.

LukasNickel · 2021-05-27T10:50:08Z

It looks like the tests are not run at all on the CI.
Are there no known CameraDescriptions?

maxnoe · 2021-05-27T10:52:09Z

See #1641

You need to use the camera_geometries fixture to make sure the files are downloaded.

maxnoe · 2021-05-27T10:55:31Z

You can actually parameterize fixtures, so maybe we should add a camera_geometry parameterized fixture these test can use:
https://docs.pytest.org/en/6.2.x/fixture.html#fixture-parametrize

maxnoe · 2021-05-27T10:58:37Z

So instead of

camera_names = CameraGeometry.get_known_camera_names()

@pytest.parametrize("camera_name", camera_names)
def test_foo(camera_name):
    camera = CameraGeometry.from_name(camera_name)

this should work better now:

from ctapipe.instrument import CameraGeometry
import pytest


@pytest.fixture(params=["LSTCam", "CHEC", "FlashCam", "NectarCam", "MAGICCam", "FACT"])
def camera_geometry(request):
    return CameraGeometry.from_name(request.param)

def test_foo(camera_geometry):
    print(len(camera_geometry))

Result:


test_fixture.py::test_foo[LSTCam] PASSED                                                                                               [ 16%]
test_fixture.py::test_foo[CHEC] PASSED                                                                                                 [ 33%]
test_fixture.py::test_foo[FlashCam] PASSED                                                                                             [ 50%]
test_fixture.py::test_foo[NectarCam] PASSED                                                                                            [ 66%]
test_fixture.py::test_foo[MAGICCam] PASSED                                                                                             [ 83%]
test_fixture.py::test_foo[FACT] PASSED

LukasNickel · 2021-05-27T11:06:00Z

Nice!
Since we actually have these defined in conftest.py already, its probably best to use them everywhere?
I will try to replace it in other tests as well

Edit: I skipped it on the other tests because the relevant tests explicitly test from_name and the known camera descriptions in some cases, so this fixture couldn't directly provide the needed info anyway.

ctapipe/image/__init__.py

kosack · 2021-05-27T16:04:40Z

By the way, if you want to make larger changes to the API, that is also fine - this code was used a lot in the studies on wavelet cleaning for example, but those already no longer work with with the latest ctapipe and are being re-written, so right now there is no real constraint on the API and a nicer one would be welcome

- Use parameterized fixture instead of known camera descriptions, because these are not cached at this point and the tests would just skip - Compare to pixel shape enum instead of string

- hexe -> hex

LukasNickel · 2021-05-27T16:19:35Z

By the way, if you want to make larger changes to the API, that is also fine

I would like to, but right now I am not sure what the best way would be. At that point it would probably also be good to have a look into the efforts of the ML group as you mentioned above. Probably not worth to rethink this twice.
In my opinion this would best be added to the CameraGeometry so that the user does not need to worry about pixel shapes at all, but there might be better places still.

review-notebook-app · 2021-05-27T16:19:45Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

kosack · 2021-05-28T08:50:49Z

We could maybe add convenience functions to CameraGeometry as you suggest, something like:

geom_2d = geom.to_2d(method='skew')
geom_1d = geom_2d.to_1d()

That just calls these functions in the background. In the future we would implement more methods or use the ones by the ML group. The to_1d() would need to know what method was used originally and should only work on 2D CameraGeometries, so maybe we would need a subclass CameraGeometry2D as the return type. That can come with another future PR if you prefer though.

ctapipe/image/geometry_converter_rect.py

ctapipe/instrument/camera/geometry.py

…_converter

maxnoe · 2021-06-30T07:57:31Z

A Cartesian image is orthogonal and has regular pixel spacing, so geom.image_{to,from}_cartesian(image) would also work

The problem I have with cartesian is that it uses opposite axis (y positive up) direction for the y-axis than the usual image convention (y positive down).

What do we do here? Does "imshow" look like the same?

kosack · 2021-06-30T08:59:17Z

What do we do here? Does "imshow" look like the same?

The actual projection (how x/y map to pixels) probably doesn't matter, it should just be documented. Imshow in fact puts Y on the Y axis, but positive down, unlike a normal plot (unless you add an option to flip it), so even using imshow doesn' give what one would expect. In the end machine learning and cleaning algorithms don't really care I guess.

LukasNickel · 2021-06-30T09:34:25Z

What do we do here? Does "imshow" look like the same?

Currently the axes are set up in a way, that imshow matches the CameraDisplay in terms of orientation (for square pixels at least). I found this to be the most natural way of comparing the images.
But this is an argument against carthesian for sure.

Naming things is hard...

Edit: Actually... Is the axis orientation really part of the definition of an euklidian vector space/grid/...?

kosack · 2021-06-30T16:57:12Z

By the way, imshow only defaults to y-negative, but you can have it display either way, so it doesn't matter really which way. It may be clearer to have a traditional x-y axis.

plt.imshow(image, origin="lower")

puts 0,0 at the lower left as usual. It only defaults to the other way because JPG and PNG (and most other graphics formats) use that convention, so that scan lines can be drawn from top-to-botttom. But that does makes it nicer to be able to export the files to PNG, etc.

Those are still cartesian images (in that they have an orthonormal basis. The only reason I didn't like "regular" is that the most common English definition is "typical", not "evenly spaced", so for me it was confusing.

The real problem is that we have overloaded the word "image"... we could use Cherenkov Image or Camera Image to mean the 1D ones ..

kosack · 2021-06-30T17:02:41Z

In fact, at one point I had thought of defining a CameraImage that was just a tuple of (geom, image_1d_array), but in the end didn't do it for simplicity (often you have many images, and only one geometry for example). But we could consider that, and then these conversion routines would be methods of CameraImage not CameraGeometry. But that is a larger API change

LukasNickel · 2021-06-30T17:43:05Z

The real problem is that we have overloaded the word "image"... we could use Cherenkov Image or Camera Image to mean the 1D ones ..

Side Note: We do name the peak times array image or time_image occasionally as well, so its already ambiguous.
Would you prefer representation?

image_{to_from}_cartesian_representation()

LukasNickel · 2021-06-30T17:53:07Z

In fact, at one point I had thought of defining a CameraImage that was just a tuple of (geom, image_1d_array), but in the end didn't do it for simplicity (often you have many images, and only one geometry for example). But we could consider that, and then these conversion routines would be methods of CameraImage not CameraGeometry. But that is a larger API change

It sounds excessive to create a new object for each image. If we were to perform all operations on n images at a time, I would be more in favor for this. Then all user facing functions could get this and avoid the extra geom every time.
To me this sounds like we reach the limits of what the containers can realistically do and we might need a more generic "datastore" at some point?

kosack · 2021-07-01T07:42:37Z

It sounds excessive to create a new object for each image.

Yes this was the main reason I did not do it, along with added complexity when writing the data.

ctapipe/instrument/camera/geometry.py

Co-authored-by: Maximilian Nöthe <[email protected]>

ctapipe/instrument/camera/tests/test_geometry.py

LukasNickel · 2021-07-05T09:11:00Z

Sorry @kosack I forgot to remove a now obsolete import in the last commit, which made the tests fail.

* Implement a general conversion between 1d and 2d images for rectangular pixels - Removes the old astri and chec code as this is now handled in the general case * Fix test, use general converter instead of astri/chec functions - Test now compares the original image to the image after tranforming back to 1d - Fixes loop, removes return: Previously most geometries were actually untested, because the pixel shape was compared to a string instead of the enum. This printed a warning about unsupported camera geometries but that was easily missed. * Add docstrings * Fix flat image dtype * Fix tests - Use parameterized fixture instead of known camera descriptions, because these are not cached at this point and the tests would just skip - Compare to pixel shape enum instead of string * Rename hexagonal geometry converter - hexe -> hex * Fix codacy complaints * Cache row and col for 2d transformation * Remove unused import * Rename row/col properties * Remove import * Move image conversion code to the camera module - Removes some hex-specific code - Why? To attach the conversion code to the camera geometry * Move tests aswell * Add functions to convert images from 1d arrays to 2d and the other way around - define pixel row and column for hexagonal pixels - Use the same, simple 1D<->2D functions for both, square and hexagonal pixels - TODO: This fails for geometries without evenly sized pixels! This probably means the old hex code needs to be added again as workaround for this case as it was working fine that way. * Calculate rows and columns of hexagonal pixels correctly - move row and column properties to one new property to avoid having to perform the same calculation twice - This fixes the case of hexagonal pixels of differing size, that was previously broken (Whipple490 geometry) * Transform multiple images at once - `to_regukar_image` and `regular_image_to_1d` now support the transformation of multiple images at once. For multiple images, the leading dimension has to be one separating the images * Clean up comments and docstrings * Remove empty notebook and unused import * Fix codacy complaints * Fix image orientation - for geometries with hexagonal pixels, the 2d image was flipped. This fixes it in the sense, that it now matches what you expect when comparing with the CameraDisplay * Update docs - Conversion of images to 2d arrays is now done inside of the camera geometry * Remove orphaned reference and add notebook for 2d image vonversion to the index * Add missing import * Rename functions for conversion between 1d and 2d image arrays * Treat circular pixels like hexagonal ones Co-authored-by: Maximilian Nöthe <[email protected]> * Remove unused test code Co-authored-by: Maximilian Nöthe <[email protected]>

LukasNickel added 4 commits May 26, 2021 11:32

Implement a general conversion between 1d and 2d images for rectangular

7d6992a

pixels - Removes the old astri and chec code as this is now handled in the general case

Add docstrings

102f3c7

Fix flat image dtype

0eca3fa

kosack reviewed May 27, 2021

View reviewed changes

ctapipe/image/__init__.py Outdated Show resolved Hide resolved

LukasNickel added 2 commits May 27, 2021 18:11

Fix tests

31a2f03

- Use parameterized fixture instead of known camera descriptions, because these are not cached at this point and the tests would just skip - Compare to pixel shape enum instead of string

Rename hexagonal geometry converter

4fed107

- hexe -> hex

Fix codacy complaints

4f6d12a

kosack previously approved these changes May 28, 2021

View reviewed changes

maxnoe reviewed May 28, 2021

View reviewed changes

ctapipe/image/geometry_converter_rect.py Outdated Show resolved Hide resolved

maxnoe mentioned this pull request Jun 1, 2021

Parallel tests #1740

Merged

Cache row and col for 2d transformation

c01e3f6

LukasNickel dismissed kosack’s stale review via c01e3f6 June 2, 2021 10:15

Remove unused import

0690b45

maxnoe reviewed Jun 2, 2021

View reviewed changes

ctapipe/instrument/camera/geometry.py Outdated Show resolved Hide resolved

Rename row/col properties

22bfc86

kosack previously approved these changes Jun 2, 2021

View reviewed changes

Merge branch 'master' of github.com:cta-observatory/ctapipe into rect…

da6f1df

…_converter

LukasNickel dismissed kosack’s stale review via da6f1df June 2, 2021 16:30

Remove import

9475ecf

Rename functions for conversion between 1d and 2d image arrays

e1d70c4

kosack previously approved these changes Jul 2, 2021

View reviewed changes

maxnoe reviewed Jul 2, 2021

View reviewed changes

ctapipe/instrument/camera/geometry.py Show resolved Hide resolved

maxnoe reviewed Jul 2, 2021

View reviewed changes

ctapipe/instrument/camera/geometry.py Outdated Show resolved Hide resolved

LukasNickel dismissed kosack’s stale review via b36506e July 2, 2021 09:20

Treat circular pixels like hexagonal ones

b36506e

Co-authored-by: Maximilian Nöthe <[email protected]>

kosack previously approved these changes Jul 2, 2021

View reviewed changes

maxnoe reviewed Jul 2, 2021

View reviewed changes

ctapipe/instrument/camera/tests/test_geometry.py Outdated Show resolved Hide resolved

LukasNickel dismissed kosack’s stale review via 5df56f2 July 2, 2021 12:53

kosack previously approved these changes Jul 2, 2021

View reviewed changes

LukasNickel dismissed kosack’s stale review via 4362665 July 2, 2021 17:30

LukasNickel force-pushed the rect_converter branch from 5df56f2 to 4362665 Compare July 2, 2021 17:30

Remove unused test code

d499d99

LukasNickel force-pushed the rect_converter branch from 4362665 to d499d99 Compare July 2, 2021 17:52

kosack approved these changes Jul 5, 2021

View reviewed changes

maxnoe approved these changes Jul 5, 2021

View reviewed changes

kosack merged commit 37d9015 into master Jul 13, 2021

maxnoe mentioned this pull request Dec 6, 2021

Geometry converters use from_name inside. #1688

Closed

This was referenced Jan 4, 2023

CHEC camera geometry converter classes broken for Prod4 #1139

Closed

Completely remove 2d geometries #1743

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generalize image converter for square pixels #1728

Generalize image converter for square pixels #1728

LukasNickel commented May 26, 2021

codecov bot commented May 26, 2021 •

edited

Loading

LukasNickel commented May 27, 2021

maxnoe commented May 27, 2021

maxnoe commented May 27, 2021 •

edited

Loading

maxnoe commented May 27, 2021 •

edited

Loading

LukasNickel commented May 27, 2021 •

edited

Loading

kosack commented May 27, 2021 •

edited

Loading

LukasNickel commented May 27, 2021

review-notebook-app bot commented May 27, 2021

kosack commented May 28, 2021 •

edited

Loading

maxnoe commented Jun 30, 2021

kosack commented Jun 30, 2021

LukasNickel commented Jun 30, 2021 •

edited

Loading

kosack commented Jun 30, 2021 •

edited

Loading

kosack commented Jun 30, 2021 •

edited

Loading

LukasNickel commented Jun 30, 2021

LukasNickel commented Jun 30, 2021

kosack commented Jul 1, 2021 •

edited

Loading

LukasNickel commented Jul 5, 2021

Generalize image converter for square pixels #1728

Generalize image converter for square pixels #1728

Conversation

LukasNickel commented May 26, 2021

codecov bot commented May 26, 2021 • edited Loading

Codecov Report

LukasNickel commented May 27, 2021

maxnoe commented May 27, 2021

maxnoe commented May 27, 2021 • edited Loading

maxnoe commented May 27, 2021 • edited Loading

LukasNickel commented May 27, 2021 • edited Loading

kosack commented May 27, 2021 • edited Loading

LukasNickel commented May 27, 2021

review-notebook-app bot commented May 27, 2021

kosack commented May 28, 2021 • edited Loading

maxnoe commented Jun 30, 2021

kosack commented Jun 30, 2021

LukasNickel commented Jun 30, 2021 • edited Loading

kosack commented Jun 30, 2021 • edited Loading

kosack commented Jun 30, 2021 • edited Loading

LukasNickel commented Jun 30, 2021

LukasNickel commented Jun 30, 2021

kosack commented Jul 1, 2021 • edited Loading

LukasNickel commented Jul 5, 2021

codecov bot commented May 26, 2021 •

edited

Loading

maxnoe commented May 27, 2021 •

edited

Loading

maxnoe commented May 27, 2021 •

edited

Loading

LukasNickel commented May 27, 2021 •

edited

Loading

kosack commented May 27, 2021 •

edited

Loading

kosack commented May 28, 2021 •

edited

Loading

LukasNickel commented Jun 30, 2021 •

edited

Loading

kosack commented Jun 30, 2021 •

edited

Loading

kosack commented Jun 30, 2021 •

edited

Loading

kosack commented Jul 1, 2021 •

edited

Loading