Add use of WCS units to center and zoom functions by confluence · Pull Request #116 · CARTAvis/carta-python

confluence · 2023-05-17T19:33:00Z

This PR implements #56, #57, and #109.

Image.set_center now accepts either world coordinates or image coordinates.
- The validation permits a wider range of formats than the GUI pan and zoom dialog, including those requested in the example snippet in allowing world coordinates as input in Image.set_center() #56. These formats are normalized to the forms accepted by the frontend. Numbers or numeric strings without units are assumed to be in degrees, for consistency with the GUI.
- A limitation is that the parameter formats must match the currently set overlay number formats (this is required by the frontend). We can detect a mismatch, but we can't automatically correct the settings because 1) we can't tell if a HMS string is supposed to be HMS or DMS, 2) we can't deduce a coordinate system from a format, and 3) we don't know if the user intends to change the coordinate system or set a custom number format. So it has to be the user's responsibility to ensure this.
- set_center does have an optional parameter to set the system (for consistency with the dropdown in the GUI dialog), and I have added high-level functions to Session to make setting or clearing a custom number format more user-friendly.
- Another limitation is that image and world coordinates can't be combined. We detect this and fail explicitly.
- We also fail if world coordinates are provided and the image has no WCS info.
The old Image.set_zoom has been renamed to Image.set_zoom_level.
Image.zoom_to_size_x and Image.zoom_to_size_y are used to fit the specified dimension to the size provided, and accept both pixel sizes and angular sizes.
- We accept a wider range of unit names than the frontend, and normalize them (to " for arcsec, ' for arcmin, and deg for degrees). A number or numeric string with no units is assumed to be in arcsec (for consistency with the frontend).
- We fail if an angular size is provided and the image has no WCS info.
This PR also contains unit tests for the high-level functionality added in this PR, as well as a basic framework for writing unit tests with pytest. Once this is merged, tests for existing high-level functions can be filled in.
- The unit test directory has been added to the format scripts.
CoordinateSystem enum values have been corrected to match the frontend.
The String validation class now has an integer parameter for arbitrary regex flags instead of a boolean parameter for ignoring the case. Existing code was using the parameter like this already (it worked by accident!).
Macro objects can now be compared (needed for unit tests).
Utility and validation classes for parsing and validating sizes and coordinates have been added and can be reused in other functions.

I made some decisions about the allowed / disallowed input formats which are up for discussion -- these can be changed relatively easily. For example:

the full list of allowed units and formats
what numbers without units default to in each function
spaces not being allowed before ' and ", but being allowed before multi-character unit words
h/d, m and s are all optional in hms / dms format. All three colons must be present if the colon separator format is used (this is consistent with the frontend). If letters are used, both number and letter must be included or excluded together. This means that "" is a valid coordinate which is evaluated as "::".
More generally, you can see what inputs are allowed by looking at the validation tests, and how they are transformed before being sent to the frontend by looking at the util tests.

To be completed:

Sample validation class tests
Session function for retrieving number format
Tests for this function and the function for retrieving the coordinate system
Basic e2e sanity check
Check the rendering of the documentation

…_units

…stom number format; added basic test framework and simple example tests in preparation for tests of these functions

…parators

I-Chenn

The new added functions work without any problem in my attempt of test.

confluence · 2023-05-27T00:27:44Z

I wasn't entirely happy with the way that the parsing of the different formats was arranged internally, so I did a bit of refactoring to clean up the design and make the separation and code reuse more logical. The user-facing API is unchanged. While updating the tests I found some with duplicate names which I think never ran, and fixed them.

kswang1029

@confluence I just had some initial code comments. Will proceed user testing.

kswang1029 · 2023-05-29T06:16:43Z

        """
        self.call_action("overlayStore.global.setSystem", system)

+    def coordinate_system(self):


shall we make it clear between native coordinate system (as defined in header) and "rendered" coordinate system?

This is mostly unambiguous because it's a function on the session, not the image (and will probably soon move to a separate image viewer class). But we can explain this in more detail in the docstring.

kswang1029 · 2023-05-29T06:25:46Z

+class AngularSizeString:
+    """Parses angular sizes."""
+
+    NORMALIZED_UNIT = {


I suggest we also support

milliarcsec (10^-3 arcsec for VLA/ALMA etc)

microarcsec (10^-6 arcsec for VLBI, EHT etc)

This is doable; I'll need to do a bit of refactoring to normalize these to one of the units accepted by the frontend. Are there any other aliases of these unit names that you would like to support?

I added these (together with the full words, and mas and µas). I also added uas, since I know that u has sometimes been used as an ASCII approximation for µ. It does turn up in search results, but possibly most of these results are just badly OCRed academic papers. If nobody actually uses this, I can take it out.

kswang1029 · 2023-05-29T06:26:31Z

+    """Parses angular sizes."""
+
+    NORMALIZED_UNIT = {
+        "arcmin": "'",


we can consider to also support arcsecond and arcminute. Sometimes, we also use "asec" for arcsec and "amin" for arcmin.

These are all very easy to add.

I added these, together with "arcseconds" and "arcminutes" ("degrees" is also supported, but only these full words can be plural, not any of the abbreviations).

kswang1029 · 2023-05-29T06:33:44Z

+    }
+
+    @classmethod
+    def valid(cls, value):


wonder if we need to validate value range? For example, for "hms" convention, 0 <= h < 24, 0 <= m < 60, and 0 <= s < 60. For "dms", -90 <= d <= 90,0 <= m < 60, and 0 <= s < 60. For "d" as "longitude", 0 <= d < 360 and as "latitude" -90 <= d <= 90

Also doable. We would need to be aware of both the coordinate system and which coordinate is which, so this probably needs to be done inside the set_center function.

As far as I can tell, the frontend currently doesn't do any validation beyond the format -- it attempts to transform whatever values are provided and applies the result if it's valid.

Actually, for HMS and DMS we can do a more specific check in the util class. We'll just need the system and the axes for the degree ranges.

@kswang1029:

Should the latitude range for degrees not be -90 <= d <= 90?

For HMS and DMS, I assume that e.g. H can only be 24 if M and S are zero (i.e. 24:12:34.5 = not valid). Is that correct?

What if the user sets DMS as the custom number format for a longitude axis? I understand that this is not the convention, but nothing is preventing it. Should we then validate that 0 <= d < 360, as for decimal degrees?

Am I correct in assuming that longitude = right ascension = x axis and latitude = declination = y axis in all coordinate systems except for ecliptic in which case latitude = x axis and right ascension = y axis? And does it still make sense for the user to provide the coordinates in (x, y) order in the latter case?

@kswang1029:

Should the latitude range for degrees not be -90 <= d <= 90?

When the coordinate system is galactic or ecliptic, we express the axis as Galactic/Ecliptic longitude and Galactic/Ecliptic latitude. Mostly, we use decimal degrees as the expression of longitude and latitude. For longitude, the range is 0 deg <= longitude < 360 deg, and for latitude, the range is -90 deg <= latitude <= 90 deg.

For HMS and DMS, I assume that e.g. H can only be 24 if M and S are zero (i.e. 24:12:34.5 = not valid). Is that correct?

Correct, the HMS (hour-minute-second) range is like 0h:0m:0s, 0h:0m:1s, ..., 23h59m59.999999....s. The DMS (degree-arcmin-arcsec) range is -90d:0m:0s, ... -89d0m0s, ..., +90d0m0s. These are all sexagesimal.

What if the user sets DMS as the custom number format for a longitude axis? I understand that this is not the convention, but nothing is preventing it. Should we then validate that 0 <= d < 360, as for decimal degrees?

Usually for ICRS, FK5 and FK4 (ie equatorial system), we use the HMS-DMS convention (sexagesimal). But it is totally fine to convert sexagesimal format to decimal degree format. For GALACTIC and ECLIPTIC, we use decimal degrees convention. Rarely we use sexagesimal format for these two systems (but it is valid to do such conversion).

Am I correct in assuming that longitude = right ascension = x axis and latitude = declination = y axis in all coordinate systems except for ecliptic in which case latitude = x axis and right ascension = y axis? And does it still make sense for the user to provide the coordinates in (x, y) order in the latter case?

AST has its logic on where to map the longitude/latitude or RA/DEC axes to the canvas xy axes if the field has extra rotation. Assuming there is no extra field rotation, for ICRS, FK5 and FK4 systems, North is up and East is left. Then x is RA and y is DEC. For GALACTIC and ECLIPTIC systems without extra field rotation, North is up and East is left too. So x is longitude and y is latitude. The tricky part is, if we select GALACTIC grid to render from a native FK5 system (or vice versa), we will see the grid is rotated (hence we may see unexpected longitude/latitude to x/y mapping due to AST).

Following up on our previous conversation, I also checked what happens in the GUI if you select HMS as the format for latitude. This appears to be valid, and negative values are converted to the expected negative HMS values, which suggests that in this case the range should be -6:00:00 to 6:00:00.

Hmmm I would suggest we fix this behavior of the frontend. 🤔 showing HMS for latitude is not right.

That may be tricky. The preferences are for the X and Y axis, and as we discussed, which axis is which depends on how AST decides to display that image.

Fortunately I was overthinking this (I misread this part of your reply). I see now that the orientation of X and Y also changes, so X is always longitude and Y is always latitude. That should make it easier to implement the ranges (and also to disable specific custom number options in the frontend).

I also misread the HMS range -- I see that 24 is excluded from the hour range.

kswang1029 · 2023-05-29T06:40:48Z

+    mock_call_action.assert_called_with("setCenter", float(x), float(y))
+
+
+@pytest.mark.parametrize("x,y,x_fmt,y_fmt,x_norm,y_norm", [


might be good to add test cases like (30d, -30d) where the latitude coordinate is a negative number. Need unit tests for this case too.

This would probably be in the unit test for set_center.

kswang1029 · 2023-05-31T04:34:56Z

@confluence here is one issue I spotted. If we have two images loaded (not matched) and they both have equatorial coordinate system defined as the native one, then if we try to set_center using GALACTIC coordinate to the non active image, the result will be incorrect. Example script is shown below (let me know if you need the two test images I used).

from carta.session import Session
from carta.constants import CoordinateSystem
import time

url_with_token = 'http://localhost:3002'
session_id = 3055419803

image_file_3 = '/Users/kswang/carta_image_pool/set_herschel_pacs_spire/spire500_ext.fits'
image_file_1 = '/Users/kswang/carta_image_pool/set_HD163296/HD163296_C18O_2-1.fits'


##############################################################

session = Session.interact(url_with_token, session_id, debug_no_auth=True)
img0 = session.open_image(image_file_3)
img1 = session.append_image(image_file_1)


print(img0.valid_wcs)
img0.set_center("7:09:00", "-11:12:37")
img0.set_zoom_level(2.0)

time.sleep(5)

img0.make_active() # try to comment out this to see the issue

img0.set_center(224.791, -1.733, system=CoordinateSystem.GALACTIC)
img0.set_zoom_level(4.0)


time.sleep(5)

session.save_rendered_view('test.png')
session.close()

Screen.Recording.2023-05-31.at.12.34.18.mov

If we make image active first before set_center, it works. However, the axis labels are not rendered correctly.

Screen.Recording.2023-05-31.at.12.36.01.mov

confluence · 2023-06-05T10:34:37Z

I'm temporarily moving this back to draft so that I can push some incomplete changes while I'm refactoring. I will move it back out once it's ready for review again.

…simal formats

…if used separately. Updated tests.

kswang1029 · 2023-06-08T02:27:51Z

+            # Image coordinates
+            x_value = PixelValue.as_float(str(x))
+            y_value = PixelValue.as_float(str(y))
+            if 0 <= x_value < self.width and 0 <= y_value < self.height:


This is minor but I just realized, in CARTA we refer the center of the pixel at the bottom-left corner as (0, 0). This means, for example in x direction we have 100 pixels, the valid range should be -0.5 <= x < 99.5 (or -0.5 <= x <= 99.5?).

I decided to make the range symmetrical -- if the user can center the bottom-left corner, they should also be able to center the other corners.

Does it make sense to apply a range limit at all? The user can specify WCS coordinates which are outside the image area, right? Should they not be able to do the same here?

Right, world coordinate can be outside the image. So guess we can remove the range check for consistency and potentially enable some use cases.

kswang1029 · 2023-06-08T02:33:18Z

+            self.call_action("zoomToSizeX", PixelValue.as_float(size))
+        else:
+            if not self.valid_wcs:
+                raise ValueError("Cannot parse angular size. This image does not contain valid WCS information.")


IIRC, if the image is not have a valid WCS, we render the xy axes as "linear". So maybe we can add a hint to use image coordinate (ie pixel) as the size input.

kswang1029 · 2023-06-08T02:38:20Z

+            self.call_action("zoomToSizeXWcs", str(AngularSize.from_string(size)))
+
+    @validate(Size())
+    def zoom_to_size_y(self, size):


Wonder if we can combine zoom_to_size_x and zoom_to_size_y as zoom_to_size and have an argument as "direction" (or "dimension" or "axis"?) for x or y? This would simplify the code repeatition here.

Good idea. I already added a SpatialAxis enum for the set_center range validation, so this is a simple change.

… of coordinates to middle of pixel

…s, for consistency with WCS coordinates

confluence · 2023-07-03T10:13:48Z

As discussed, I will merge this as-is so that we can start using the testing framework, and create a new issue for the inactive image problem.

confluence added 11 commits April 8, 2023 11:25

Notes

4cabe3e

Merge remote-tracking branch 'origin/dev' into confluence/zoom_centre…

5199c1d

…_units

First pass of implementation. Needs testing.

c534dad

Some fixes and revisions; added functions for setting and clearing cu…

6bd7dca

…stom number format; added basic test framework and simple example tests in preparation for tests of these functions

improved property mocking

ffe2837

some basic set_center tests

f7d4a3b

more consistent type coercion

a3dddfe

fixed bug; completed set_center tests

562869d

More tests; fixed some bugs

0b47f84

Added and refined tests

e1f5a39

Added some session tests

9d0fd14

confluence marked this pull request as draft May 17, 2023 19:33

confluence added 7 commits May 17, 2023 22:17

The actual correct CoordinateSystem values

4e970ff

Number format function; more tests

3bf31f0

Added validation tests and corrected logic for hms/dms with letter se…

d1ca655

…parators

Fixed some documentation issues

9f55cca

bumped version

3ce9b20

Arguments to non-WCS zoom and center actions must be numbers

c2ffbe9

Fixed function names

a66d241

confluence marked this pull request as ready for review May 18, 2023 10:36

confluence self-assigned this May 18, 2023

confluence requested review from I-Chenn, acdo2002 and kswang1029 May 18, 2023 10:36

number_format should wrap return values in NumberFormat enums

a795416

I-Chenn approved these changes May 25, 2023

View reviewed changes

Refactored internal implementation; fixed tests

2433ae2

kswang1029 requested changes May 29, 2023

View reviewed changes

confluence added 7 commits June 1, 2023 23:12

partial refactoring in preparation for range checks

6c3a6e6

Rename object tree

cc3d67f

rename angular size class

cd337f4

Renamed and refactored pixel value class; fixed tests

ecb80ab

Simplified AngularSize and WorldCoordinate usage

094df0d

Refactored angular size class

c9d5da6

Added support for unicode arcmin and arcsec symbols.

195a6b4

I-Chenn self-requested a review June 2, 2023 03:12

Refactoring and test rewrites

fd7924d

confluence marked this pull request as draft June 5, 2023 10:33

confluence added 4 commits June 5, 2023 12:44

Finish refactoring tests

d7b56c2

Restored previously removed support for empty H, M and/or S in sexage…

0cb42a0

…simal formats

Refactored angular size classes so that child classes work correctly …

db25e2e

…if used separately. Updated tests.

Finished refactoring angular size tests

8f240bf

confluence marked this pull request as ready for review June 5, 2023 20:03

kswang1029 reviewed Jun 8, 2023

View reviewed changes

confluence added 4 commits June 8, 2023 16:04

Shift permitted range for set_center pixel values to allow for offset…

09b9ef9

… of coordinates to middle of pixel

Added hint to error message about missing WCS.

6bf54f8

Combined zoom_to_size_* functions into one function with axis parameter

57988b2

Remove range validation entirely for set_center with image coordinate…

7666c36

…s, for consistency with WCS coordinates

confluence merged commit d7a1e9d into dev Jul 3, 2023

confluence deleted the confluence/zoom_centre_units branch July 3, 2023 10:14

confluence mentioned this pull request Jul 3, 2023

Setting the coordinate system and then the centre position doesn't work correctly if the image is not active #125

Open

This was referenced Jul 12, 2023

allowing world coordinates as input in Image.set_center() #56

Closed

allowing angular size as input in Image.set_zoom() #57

Closed

Add a framework for high-level function unit tests #109

Closed

		mock_call_action.assert_called_with("setCenter", float(x), float(y))


		@pytest.mark.parametrize("x,y,x_fmt,y_fmt,x_norm,y_norm", [

Conversation

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

Uh oh!

Uh oh!

I-Chenn left a comment

Choose a reason for hiding this comment

Uh oh!

confluence commented May 27, 2023

Uh oh!

kswang1029 left a comment

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!

kswang1029 May 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

confluence May 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

kswang1029 Jun 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

confluence Jun 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

confluence Jun 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

kswang1029 commented May 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

confluence commented Jun 5, 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!

confluence commented Jul 3, 2023

confluence commented May 17, 2023 •

edited

Loading

kswang1029 May 29, 2023 •

edited

Loading

confluence May 29, 2023 •

edited

Loading

kswang1029 Jun 1, 2023 •

edited

Loading

confluence Jun 1, 2023 •

edited

Loading

confluence Jun 2, 2023 •

edited

Loading

kswang1029 commented May 31, 2023 •

edited

Loading