Refactor and unify the interface for the raster image, contours, and vector overlay.

[I-Chenn]

The PR is to resolve #58.

Move bias and contrast from Image.set_colormap() to Image.set_scaling(). Will wait for the merge of #79 to accept the AUTO value.

[confluence]

@kswang1029 Sorry about the delay in getting back to this. I'm going to investigate the unresolved issues from your list. In some cases I'm going to create new issues, as some of these bugs are outside the scope of the refactoring in this PR (and I would like to get these large PRs merged in to make further testing and development easier).

[kswang1029]

@kswang1029 Sorry about the delay in getting back to this. I'm going to investigate the unresolved issues from your list. In some cases I'm going to create new issues, as some of these bugs are outside the scope of the refactoring in this PR (and I would like to get these large PRs merged in to make further testing and development easier).

No problem at all. I should be able to be back to work next week and will catch up debt asap.

[confluence]

@kswang1029 since the beam is actually purely a per-image property, and not a global setting, I'm planning to move it out of the wcs object entirely and make it accessible through the image object (img1.beam, img2.beam, etc.). Does that make sense?

[kswang1029]

@kswang1029 since the beam is actually purely a per-image property, and not a global setting, I'm planning to move it out of the wcs object entirely and make it accessible through the image object (img1.beam, img2.beam, etc.). Does that make sense?

@confluence I think we also have the same problem with the title rendered by AST? We should be able to specify different titles for different images. The behavior then would be the same as the rendered beam elements. In my opinion, having controls of title and beam in the WCSOverlay class feels for unified and consistent (to other AST props), despite the beam element is not rendered via AST.

Perhaps we can consider the following?
session.wcs.ticks etc
session.wcs.title
session.wcs.colorbar
session.wcs.beam

Or not sure if we want to introduce the idea of a "canvas" where it keeps wcs (all the AST props, including title), panel layout props, colorbar, and beam, in order to make the relationships unambiguous?

[confluence]

@confluence I think we also have the same problem with the title rendered by AST? We should be able to specify different titles for different images. The behavior then would be the same as the rendered beam elements. In my opinion, having controls of title and beam in the WCSOverlay class feels for unified and consistent (to other AST props), despite the beam element is not rendered via AST.

@kswang1029 we already have separate image functions for changing the per-image title and colorbar label. I was going to argue, but you've convinced me that all of these functions should be accessible through the session's wcs object for consistency. I would, however, like to add / keep the functions on the image object as well, since I strongly feel that it's the place that per-image settings are expected to be, and if the user already has an image object they should be able to use that object to access these settings (and not have to get the ID and pass it as a parameter to a function on the session).

This is analogous to the export function in the regions PR: I added both a per-region export_to function, and a regions.export_to function which acts on any subset of regions, depending on a parameter (defaulting to all regions).

1. Do you think that per-image WCS settings on the image would feel more consistent if they were grouped under a wcs subobject on the image? So for example: img1.wcs.beam.set_type(...) and img1.wcs.title.set_text("something")?

2. Is the user ever likely to want to set the same value on multiple images at once for any of these properties? It seems possible for the beam colour, width, and visibility, and very unlikely for the title text. What about the beam type and position, or the colorbar label? This will determine if these functions act on all images by default, or require an image parameter. It probably doesn't make sense ever to default to the active image, since we're moving away from the concept of an active image in the wrapper.

[kswang1029]

@confluence I think we also have the same problem with the title rendered by AST? We should be able to specify different titles for different images. The behavior then would be the same as the rendered beam elements. In my opinion, having controls of title and beam in the WCSOverlay class feels for unified and consistent (to other AST props), despite the beam element is not rendered via AST.

@kswang1029 we already have separate image functions for changing the per-image title and colorbar label. I was going to argue, but you've convinced me that all of these functions should be accessible through the session's wcs object for consistency. I would, however, like to add / keep the functions on the image object as well, since I strongly feel that it's the place that per-image settings are expected to be, and if the user already has an image object they should be able to use that object to access these settings (and not have to get the ID and pass it as a parameter to a function on the session).

This is analogous to the export function in the regions PR: I added both a per-region export_to function, and a regions.export_to function which acts on any subset of regions, depending on a parameter (defaulting to all regions).

1. Do you think that per-image WCS settings on the image would feel more consistent if they were grouped under a wcs subobject on the image? So for example: img1.wcs.beam.set_type(...) and img1.wcs.title.set_text("something")?

Yes.

2. Is the user ever likely to want to set the same value on multiple images at once for any of these properties? It seems possible for the beam colour, width, and visibility, and very unlikely for the title text. What about the beam type and position, or the colorbar label? This will determine if these functions act on all images by default, or require an image parameter. It probably doesn't make sense ever to default to the active image, since we're moving away from the concept of an active image in the wrapper.

Usually we apply same props for the beam in different panels. However, there are chances that different panels need different beam props due to the image feature distributions (to avoid blocking). When there are matched images (rendered as a raster and contours, for example), we may need to fine tune where and how the corresponding beams are rendered. Maybe we should fetch the default settings from the frontend (or the preferences.json) and apply them as the default per image. Then for each image, users can have flexibility to change props. And yes, we better move away from the concept of an active image in the scripting interface.

[confluence]

@kswang1029 I have refactored the per-image WCS overlay properties. They are now accessible both from the session object through the existing wcs subobject tree (with additional image ID parameters), and through a dedicated wcs subobject tree on each image object (which contains only image-specific properties). All of the global functions for accessing per-image properties take an iterable of IDs and default to all images.

(I initially made the the custom text setters and getters take a single required ID parameter, but as I was writing this explanation I decided that it would be less confusing and more consistent for all of the functions to have the same interface, regardless of how likely the user is to set the same value on multiple images at once.)

So if you want to set the image title text or the colorbar label text, you do either this:

session.wcs.title.set_text("My Title", [2])
session.wcs.colorbar.label.set_text("My Label", [1])

...or this:

img2.wcs.title.set_text("My Title")
img1.wcs.colorbar.label.set_text("My Label")

If you want to set the color of all the beams, one of the beams, or some of the beams, you can do this:

session.wcs.beam.set_color(PaletteColor.BLUE) # all of the images
session.wcs.beam.set_color(PaletteColor.BLUE, [1]) # just image 1
session.wcs.beam.set_color(PaletteColor.BLUE, [2, 3]) # images 2 and 3

...or you can do this per image:

img2.wcs.beam.set_color(PaletteColor.BLUE) 

The global wcs property getters mirror the global property setters, and all return tuples of values (one per image):

session.wcs.beam.color() # all of the images
session.wcs.beam.color([1]) # just image 1
session.wcs.beam.color([2, 3]) # images 2 and 3

session.wcs.title.text() # all of the images
session.wcs.title.text([1]) # just image 1
session.wcs.title.text([2, 3]) # images 2 and 3

...whereas the image wcs property getters mirror their setters, are attributes rather than methods, and all return single values:

img1.wcs.beam.color
img2.wcs.title.text

[kswang1029]

@kswang1029 I have refactored the per-image WCS overlay properties. They are now accessible both from the session object through the existing wcs subobject tree (with additional image ID parameters), and through a dedicated wcs subobject tree on each image object (which contains only image-specific properties). All of the global functions for accessing per-image properties take an iterable of IDs and default to all images.

(I initially made the the custom text setters and getters take a single required ID parameter, but as I was writing this explanation I decided that it would be less confusing and more consistent for all of the functions to have the same interface, regardless of how likely the user is to set the same value on multiple images at once.)

So if you want to set the image title text or the colorbar label text, you do either this:

session.wcs.title.set_text("My Title", [2])

session.wcs.colorbar.label.set_text("My Label", [1])

...or this:

img2.wcs.title.set_text("My Title")

img1.wcs.colorbar.label.set_text("My Label")

If you want to set the color of all the beams, one of the beams, or some of the beams, you can do this:

session.wcs.beam.set_color(PaletteColor.BLUE) # all of the images

session.wcs.beam.set_color(PaletteColor.BLUE, [1]) # just image 1

session.wcs.beam.set_color(PaletteColor.BLUE, [2, 3]) # images 2 and 3

...or you can do this per image:

img2.wcs.beam.set_color(PaletteColor.BLUE) 

The global wcs property getters mirror the global property setters, and all return tuples of values (one per image):

session.wcs.beam.color() # all of the images

session.wcs.beam.color([1]) # just image 1

session.wcs.beam.color([2, 3]) # images 2 and 3



session.wcs.title.text() # all of the images

session.wcs.title.text([1]) # just image 1

session.wcs.title.text([2, 3]) # images 2 and 3

...whereas the image wcs property getters mirror their setters, are attributes rather than methods, and all return single values:

img1.wcs.beam.color

img2.wcs.title.text

This approach is great and quite flexible for different use cases! 🚀

[confluence]

I originally omitted the pixel grid and spectral conversion from the WCS overlay object because they aren't WCS properties (in the same way that the pan and zoom commands are currently methods on the image object, even though in the GUI they're in a tab in the same configuration dialog as the WCS properties.

My current proposal is to put the pixel grid commands in a "raster" object on the session (because the pixel grid is a global setting which affects how image raster components are rendered, and it disappears/reappears when you hide/show an image's raster component), and to put the spectral conversion methods directly on the image object (because they're per-image settings, and this is consistent with the location of the pan and zoom controls, channel / stokes navigation, etc.). But I'm open to suggestions.

Usage (pixel grid):

session.raster.show_pixel_grid()
session.raster.set_pixel_grid_color(PaletteColor.BLUE)
session.raster.set_pixel_grid(visible=True, color=PaletteColor.BLUE)

Usage (spectral conversion):

img.set_spectral_system(SpectralSystem.LSRK)
img.set_spectral_coordinate(SpectralType.FREQ, SpectralUnit.HZ)
img.set_spectral_coordinate(SpectralType.FREQ) # uses default frequency unit of GHz
img.set_spectral_coordinate("FREQ", "Hz") # bare strings can also be used, but they have to match the enum string values

There currently isn't one command for setting both properties in one step, but I could add one if it would be useful.

[kswang1029]

I originally omitted the pixel grid and spectral conversion from the WCS overlay object because they aren't WCS properties (in the same way that the pan and zoom commands are currently methods on the image object, even though in the GUI they're in a tab in the same configuration dialog as the WCS properties.

My current proposal is to put the pixel grid commands in a "raster" object on the session (because the pixel grid is a global setting which affects how image raster components are rendered, and it disappears/reappears when you hide/show an image's raster component), and to put the spectral conversion methods directly on the image object (because they're per-image settings, and this is consistent with the location of the pan and zoom controls, channel / stokes navigation, etc.). But I'm open to suggestions.

Usage (pixel grid):

session.raster.show_pixel_grid()
session.raster.set_pixel_grid_color(PaletteColor.BLUE)
session.raster.set_pixel_grid(visible=True, color=PaletteColor.BLUE)

Usage (spectral conversion):

img.set_spectral_system(SpectralSystem.LSRK)
img.set_spectral_coordinate(SpectralType.FREQ, SpectralUnit.HZ)
img.set_spectral_coordinate(SpectralType.FREQ) # uses default frequency unit of GHz
img.set_spectral_coordinate("FREQ", "Hz") # bare strings can also be used, but they have to match the enum string values

There currently isn't one command for setting both properties in one step, but I could add one if it would be useful.

The arrangement of the methods looks fine to me.

For the img.set_spectral_system and img.set_spectral_coordinate methods, I suggest we add the docstrings to mention these are effective only for the spatial-spectral images such as a position-velocity image or a cube with permuted-axes like RA-FREQ-DEC, for example.

[confluence]

For the img.set_spectral_system and img.set_spectral_coordinate methods, I suggest we add the docstrings to mention these are effective only for the spatial-spectral images such as a position-velocity image or a cube with permuted-axes like RA-FREQ-DEC, for example.

Will do. I already have a line about this in the docstring, but I will update it to this more detailed wording.

I assumed from the frontend code that the special "native" and "channel" spectral types (for unsupported images?) are not applicable to these functions, and so omitted them. Is that correct?

[kswang1029]

For the img.set_spectral_system and img.set_spectral_coordinate methods, I suggest we add the docstrings to mention these are effective only for the spatial-spectral images such as a position-velocity image or a cube with permuted-axes like RA-FREQ-DEC, for example.

Will do. I already have a line about this in the docstring, but I will update it to this more detailed wording.

I assumed from the frontend code that the special "native" and "channel" spectral types (for unsupported images?) are not applicable to these functions, and so omitted them. Is that correct?

Correct. 👍

[confluence]

@kswang1029 I have gone through the list of issues here, and I think they have mostly been addressed.

• I just removed the "toggle labels" function; I agree that it's redundant.

• Regarding the Chrome browser colour profile, there's a separate issue for that (color profile of the chrome browser #92).

• Regarding the matched image bug: I have investigated this with multiple copies of the same image in both an interactive and a headless session, and can't reproduce it -- whether I set the center / channel / zoom through the reference image or the matched image, both images are updated. I wonder if this is the redrawing bug again, or an issue with these specific test images (if you can provide me with the images, I can check that). Either way, I think that this is also not directly related to this refactoring PR, and should be reported in a new issue.

[kswang1029]

@kswang1029 I have gone through the list of issues here, and I think they have mostly been addressed.

• I just removed the "toggle labels" function; I agree that it's redundant.

• Regarding the Chrome browser colour profile, there's a separate issue for that (color profile of the chrome browser #92).

• Regarding the matched image bug: I have investigated this with multiple copies of the same image in both an interactive and a headless session, and can't reproduce it -- whether I set the center / channel / zoom through the reference image or the matched image, both images are updated. I wonder if this is the redrawing bug again, or an issue with these specific test images (if you can provide me with the images, I can check that). Either way, I think that this is also not directly related to this refactoring PR, and should be reported in a new issue.

OK, will run e2e tests and finalize them accordingly. Almost there!

[confluence]

@kswang1029 I think the "black" palette colour issue is actually a misunderstanding: the palette colours are all predefined colours that we get from the Blueprintjs library, and their "black" has never been actual black (the real colour values differ between library versions).

If I export a screenshot from CARTA showing this as one of the overlay colours (from Firefox), and check the colour value, I get the exact same RGB value you're getting (10161a or RGB(16,22,26)), and it matches the value from several Blueprint versions ago, which I believe is what we're using in the frontend. So it looks like this is working as designed.

(Although, now that I'm looking at this, I see that I hardcoded the wrong values into the wrapper's palette-to-rgb colour conversion function, because I used the latest Blueprint version as a reference instead of the one that we're using. I will fix that in a separate PR. This doesn't affect this issue; it's a standalone function.)

[kswang1029]

Looks good. Let's move forward. I have created a set of e2e tests accordingly. Once this PR is merged, I will enable them in the e2e CI servers.

[confluence]

Great! I will merge this, and we can move onto the regions PR.