Bugfix/sci cam sign flip by steigersg · Pull Request #124 · spacetelescope/catkit2

steigersg · 2023-10-17T15:46:36Z

See affiliated PR on hicat-package2 (https://github.com/spacetelescope/hicat-package2/pull/480)

Add infrastructure in camera services to rotate and/or flip the images on the camera.

The camera rotation happens first and defines which camera axis (width or height) is aligned with the gravity vector of the testbed. Defining rot90 to be True will perform one counter-clockwise 90 degree rotation. If a clockwise rotation is desired then flip_y should also be set to True.

The flips (flip_x, and flip_y), with the exception of the example above, are meant to be defined with respect to the optical axis (e.g. whether a lens upstream of the camera has flipped the image).

Notably offset_x and offset_y values are not changed with the reflections and rotations since these are fundamentally defined with respect to the camera array and the setters and getters for these values are directly given to the setROI functions in the vendor camera control software itself. I believe this is alright since finding these values should not have to occur very frequently and the user can take care to make sure they are defined with respect to the coordinate system of the camera itself.

Tasks before review

Test on hardware (might have to do with phase retrieval camera since this is the only current one with a rotation)
Optimize for speed and memory usage

…otations

…rotations

raphaelpclt · 2023-10-17T16:02:28Z

+        make_property_helper('rot90')
+        make_property_helper('flip_x')
+        make_property_helper('flip_y')


Don't we want them to be read-only?

good look, I agree and will update both here and in the flir_camera

steigersg · 2023-10-17T18:00:07Z

@raphaelpclt brought up a good point that we currently have the functionality to update the offset_x and offset_y on the fly and these changes would result in unexpected behavior for that. We can define the offset_x and offset_y to be with respect to the transformed (rotated and/or flipped) coordinate system in the yml to preserve this functionality and then convert those values to camera array coordinates to be passed to the internal camera getROI functions. My only thoughts against this option are that we would then have two different offset_x and offset_y conventions that may lead to some confusion down the road

ehpor · 2023-10-17T19:35:13Z

@steigersg Yes, I want the offset_x and offset_y to be wrt to the transformed coordinate system. That is, I want offset_x += 10 to shift the image to the left, as pixel (10,0) is now the new top-left coordinate. This is what makes this PR more time-consuming and why it got postponed for so long.

I was for not even showing the actual offset_x/y values to outside of the service, to avoid any confusion between the hardware values and the service values. The same is true for the width and height parameters, which should be in the transformed parameter space, not the device coordinates. I think @raphaelpclt said that you wanted to have them available anyways? I don't really see a good reason why you'd wanna do this, except for direct comparison with external viewers (SharpCap, SpinView, etc...), which we almost never do anymore.

We could use a suffix _device for the device parameters, to distinguish them from the transformed, virtual coordinates.

steigersg · 2023-10-17T20:13:22Z

+            img = np.flipud(img)
+        if self.flip_y:
+            img = np.fliplr(img)
+        return img


does not result in C Contiguous array - working on it

This is expected and desired. This is what makes it fast to run multiple of these operations in a row.

ehpor · 2023-10-17T22:21:38Z

Some benchmarks for just the rot_flip_image() function (without the np.contiguousarray()). I wanna make sure this is not a significant factor in our latency numbers. Let's do a few cases, all using 1024x1024 images.

nothing:
56.3 ns ± 0.569 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

only rot90:
2.82 µs ± 54.9 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

only flip_x:
300 ns ± 6.72 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

flip_x + flip_y:
520 ns ± 8.14 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

rot90 + flip_x:
3.05 µs ± 81.8 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

copy to contiguous array with rot90:
1.13 ms ± 8.24 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)

copy to contiguous array without rot90:
617 µs ± 36.8 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)
480 µs ± 9.15 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)

So the copy takes by far the most time.

I just noticed that all these benchmarks were done with float64, not float32 dtype. But the copy takes about the same amount of time, likely because cache access in weird ordering takes the most time rather than the memory copying itself (ie. no/little acceleration from SIMD instructions or something).

Losing a full millisecond is not great, but for these large array sizes, we're not gonna run that fast anyways. For smaller array sizes (eg. 312x312, float32 as for our zernike camera), the same benchmark is
35.9 µs ± 1.04 µs per loop (mean ± std. dev. of 7 runs, 10,000 loops each)
for rot90, no flips. So that would be sufficiently fast.

ehpor · 2023-10-17T22:30:43Z

For comparison, on the same machine:

submitting 1024x1024 float32 on data stream (which copies the data internally):
73.2 µs ± 300 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each)

copying 1024x1024 float32 with array.copy():
222 µs ± 10.5 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)

ehpor · 2023-10-17T22:33:49Z

Just tested as well:

using a.T (as an alternative to rot90) with ascontiguousarray:
940 µs ± 20.7 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)

so the same speed as the rot90 for float32.

…s and the x and y offsets to apply in camera array coordinates.

ehpor · 2023-10-24T15:52:01Z

+        self.rot90 = rot90
+        self.flip_x = flip_x
+        self.flip_y = flip_y


I think you can set these straight from the config. I don't see a need to have them as local variables first. Also, this might need to be moved up to before you're actually setting offset_x/y and width/height on the camera, since they need these to exist.

ehpor · 2023-10-24T15:53:20Z

+        make_property_helper('rot90', read_only=True)
+        make_property_helper('flip_x', read_only=True)
+        make_property_helper('flip_y', read_only=True)


Do we need these as properties? Or can we have them in the CameraProxy object instead?

I don't have an intuition for what the pros and cons of having them in the CameraProxy vs. here would be. I agree though they don't necessarily have to be properties since they're only used in the rot_flip_image and get_camera_offset functions and could easily be key word arguments in there

To give some context, I'm not talking about Python properties. I'm talking about catkit2 properties, which is what the make_property_helper() function is creating. Those properties are available from the outside, ie. from client-side. Communication is done via TCP, which is not necessary. Since they are read-only and they are never meant to be changed/changable, their value can be purely gotten from the config file. Therefore, no communication between ServiceProxy and Service needs to take place, making it much easier to get the right value straight from the config in the ServiceProxy.

ehpor · 2023-10-24T15:55:12Z

+        """
+        # Define the translation matrix T to get to the center of the ROI.
+        T = np.zeros((3, 3))
+        np.fill_diagonal(T, 1)


I'd construct the matrix with np.eye(3) instead rather than creating them as zeros and then filling the diagonal with ones. The same for the other matrices.

thanks! I forgot np.eye existed - updated that code for all the matrices where I wanted diagonals filled

ehpor · 2023-10-24T16:04:02Z

+        if self.flip_x:
+            # Define x reflection matrix.
+            X[0][0] = -1
+
+        if self.flip_y:
+            # Define y reflection matrix.
+            Y[1][1] = -1


Don't the flips also need translation matrices, since the flips are reflections around the centerlines of the images?

good catch! moved the final translation to after the x and y flips

ehpor · 2023-10-24T16:06:40Z

        self.width = self.config.get('width', self.sensor_width - offset_x)
        self.height = self.config.get('height', self.sensor_height - offset_y)
+
+        offset_x, offset_y = self.get_camera_offset(offset_x, offset_y)


The properties height, width, offset_x and offset_y need to be modified instead. Otherwise, setting the offset_x and offset_y dynamically won't work as expected.

ehpor · 2023-10-24T16:07:22Z

        self.width = self.config.get('width', self.sensor_width - offset_x)
        self.height = self.config.get('height', self.sensor_height - offset_y)


Don't the width/height properties need to be changed as well? To exchange width/height that is.

if rot90 is True I updated the code so that the width and height properties are swapped. This could cause issues though if the get_camera_offset function hasn't been called yet (since it needs to know the previous origin location) so I also added an in-line comment to warn agains this.

…en applied

raphaelpclt · 2024-10-07T20:44:50Z

Should be superseded with camera base class #131

steigersg added 2 commits October 17, 2023 10:18

add infrastructure in zwo_camera service to handle camera flips and r…

d77cc6e

…otations

add infrastructure in flir_camera service to handle camera flips and …

80eccb1

…rotations

steigersg self-assigned this Oct 17, 2023

steigersg requested review from ehpor and raphaelpclt October 17, 2023 15:57

raphaelpclt reviewed Oct 17, 2023

View reviewed changes

make rot90, flip_x, and flip_y properties read only

5f191c2

steigersg commented Oct 17, 2023

View reviewed changes

hack fix for making sure array is c contiguous post-transformation

c57a239

steigersg added 4 commits October 23, 2023 11:11

define camera offset function to convert between input x and y offset…

9929bff

…s and the x and y offsets to apply in camera array coordinates.

add function call, removes TODO

d4cd568

camera offset bug fix for non-symmetric array dimensions

04976f7

adjust camera_offset function flips so they flip over proper axes

ac33100

ehpor reviewed Oct 24, 2023

View reviewed changes

steigersg added 5 commits October 24, 2023 13:04

change np.fill_diagonals to np.eye

e611147

set appropriate values for self.width and self.height if rot90 is True

dedbc6b

clean up property setting

4533d5d

update comments

cc977fe

move translation from origin to after the x and y reflections have be…

f1affe5

…en applied

ehpor mentioned this pull request Oct 26, 2023

Hardcode flip for science camera as a hack #132

Merged

raphaelpclt mentioned this pull request Oct 22, 2024

Feature/4d service #255

Merged

ivalaginja mentioned this pull request Jun 26, 2025

Add base class for cameras #131

Draft

		self.width = self.config.get('width', self.sensor_width - offset_x)
		self.height = self.config.get('height', self.sensor_height - offset_y)

Conversation

steigersg commented Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tasks before review

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steigersg commented Oct 17, 2023

Uh oh!

ehpor commented Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ehpor commented Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ehpor commented Oct 17, 2023

Uh oh!

ehpor commented Oct 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raphaelpclt commented Oct 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

steigersg commented Oct 17, 2023 •

edited

Loading

ehpor commented Oct 17, 2023 •

edited

Loading

ehpor commented Oct 17, 2023 •

edited

Loading