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 does this split of the colormap functions make sense, or do you think I should merge set_colormap and set_scaling into a single function called something like configure_raster, etc., for consistency with configure_contours and the soon-to-be-added configure_vector_overlay? Should I do that and keep the more fine-grained functions, and if yes, does it make sense to split bias/contrast into its own small function?

Other notes: I haven't added a min/max for the custom clip min and max parameters -- it appears that in the GUI you can set them to be lower or higher than the image min/max if you want. I also implemented other API changes from my comment in #58.

[kswang1029]

@kswang1029 does this split of the colormap functions make sense, or do you think I should merge set_colormap and set_scaling into a single function called something like configure_raster, etc., for consistency with configure_contours and the soon-to-be-added configure_vector_overlay? Should I do that and keep the more fine-grained functions, and if yes, does it make sense to split bias/contrast into its own small function?

How about we keep these more fine-grained functions and create a high level function configure_raster as a wrapper of those? Then users can choose. In this approach we can have consistent configure_xxx functions for raster, contour, and vector rendering.

Other notes: I haven't added a min/max for the custom clip min and max parameters -- it appears that in the GUI you can set them to be lower or higher than the image min/max if you want. I also implemented other API changes from my comment in #58.

ok. Custom clip min and max parameters in the GUI indeed can be any float.

should I proceed code review/test (or wait for the high level function configure_raster)?

[confluence]

I've been thinking about how to organise both the code and the user API to break up the huge image and session classes and group related functionality together, and I think that it would be good to split some items into sub-properties on these objects. This would make the code less unwieldy and also simplify some of the API naming. So far some groups I've identified are: the overlay (session), and the raster, contours, vector overlay, and the about-to-be added regions (image). At a later stage we will probably add other functionality that follows the same pattern. Some functionality (e.g. inspecting image geometry and other properties) I consider to be "basic" functionality which can remain accessible directly from the main object. I am considering adding an images group to the session object, for consistency with the regions group I'm planning to add to the image object (the image-opening functions would be grouped under this object).

This would change the user-facing API, so I think we should do it sooner other than later (and it may also affect the e2e tests). My suggestion would make the API look something like this:

session.overlay.set_coordinate_system(...)
session.images.open(...) # maybe?
img.raster.set_colormap(...)
img.contours.set_colormap(...)
img.vectors.set_colormap(...) # simpler than vector_overlay?
# To be added in upcoming region implementation
img.regions.add_rectangular(...)

Does that make sense?

I was considering transparently aliasing these functions to functions on the main session and image objects (for backwards compatibility), but 1) that would be a lot of boilerplate, and 2) it would complicate the naming. So I think we should commit to the backwards-incompatible change now, before the wrapper has been more widely released.

[confluence]

I'm currently working on #83, and I see that both the vector overlay and the contours have functions for setting the colourmap which also set the bias and contrast. To be consistent, we could move those properties to different functions, but those functions can't be called set_scaling because the contour and vector overlay colourmaps don't have any scaling options. I think that there are two options:

1. Move the bias and contrast for vector overlay and contour colourmaps to separate functions called set_colormap_properties and rename set_scaling to match
2. Merge set_scaling for the raster colourmap back into the raster set_colormap; leave the contour and vector overlay functions as they are; make the colourmap parameter optional so that they can be used to change the properties without changing the colourmap.

~I think that 2 is my preference, to avoid unnecessary fragmentation, and because these properties are all related to colourmaps.~ I have a better idea, and will make a longer comment below.

[confluence]

I would also rename the contour function for setting the dash properties to a more generic set_style, for consistency with the vector overlay.

[confluence]

For ease of editing I'm going to merge the branch in #83 in here before it is merged into dev.

[confluence]

@kswang1029 given that we have the all-in-one configuration functions (currently called plot), I don't think it makes sense to keep the big functions (configure, etc.) that set large numbers of properties. It's redundancy with no benefit. I would suggest that:

1. those functions are broken up into smaller, more granular functions for setting small groups of related properties: for example, in the raster component I suggest set_colormap (colormap and option to invert), set_scaling (scaling, alpha, gamma), set_clip (rank or custom min and max), set_bias_and_contrast (bias, contrast)
2. the current plot is renamed to configure
3. an all-in-one configure is added to the raster component.

(Similarly, when the overlay functions are factored out of the session object, there can be an all-in-one configure function for the overlay.)

Does this sound like a good idea?

[kswang1029]

@kswang1029 given that we have the all-in-one configuration functions (currently called plot), I don't think it makes sense to keep the big functions (configure, etc.) that set large numbers of properties. It's redundancy with no benefit. I would suggest that:

1. those functions are broken up into smaller, more granular functions for setting small groups of related properties: for example, in the raster component I suggest set_colormap (colormap and option to invert), set_scaling (scaling, alpha, gamma), set_clip (rank or custom min and max), set_bias_and_contrast (bias, contrast)
2. the current plot is renamed to configure
3. an all-in-one configure is added to the raster component.

(Similarly, when the overlay functions are factored out of the session object, there can be an all-in-one configure function for the overlay.)

Does this sound like a good idea?

@confluence sounds like a good plan. Can we still use "plot" instead of "configure"? "plot" sounds more intuitive as users use it to "plot" something explicitly. "configure" feels more implicitly. Maybe this is my own taste...

[confluence]

@confluence sounds like a good plan. Can we still use "plot" instead of "configure"? "plot" sounds more intuitive as users use it to "plot" something explicitly. "configure" feels more implicitly. Maybe this is my own taste...

My preference for renaming is so that we can use the same name for raster, vector overlay and contours. plot wouldn't make sense for the raster image, since it's already there by default. configure is more generic. But maybe a different third thing would work?

[confluence]

Actually... upon reflection, I don't think we need an all-in-one function for the raster. Contours and vectors are not enabled by default, so you need to enable them, and it's useful to specify all the settings in one step. But the raster is already enabled, so I think it's fine just to have the granular functions for changing specific settings.

So I suggest that we keep plot and don't add a function for this to raster.

[confluence]

Looking at the code again I've realised that the reason we have the configure functions in the first place is that they correspond to single functions in the frontend which have non-optional parameters. To split them up in the wrapper we'd have to replicate a lot of code for filling unused parameters with macros. We could add more functions which call configure with different subsets of parameters, but this seems like a lot of overhead with little benefit (the user can already call configure with any subset of named parameters). So I'm going to keep the configure functions as they are, and only refactor the style functions (which are where the consistency issues are anyway, because they have similar options -- the configure functions are completely different).

[confluence]

This now also resolves #78.

[kswang1029]

Regarding img.contours.set_dash(dash_mode, dash_thickness), I would suggest we break this one into two functions as img.contours.set_dash_mode(dash_mode) and img.contours.set_thinkness(thickness) so that it feels more intuitive. Mostly the default dash_mode is what astronomers want and mostly we just want to customize the rendered thickness of contours.

[kswang1029]

Few issues related to session.wcs

session.wcs.beam.type ==> CartaBadResponse: CARTA scripting action .fetchParameter called with parameters (Macro('overlayStore.beam.settingsForDisplay', 'beamType'),) expected a response, but did not receive one.

session.wcs.beam.set_type(BeamType.SOLID) ==> CartaActionFailed: CARTA scripting action overlayStore.beam.settingsForDisplay.setBeamType called with parameters (<BeamType.SOLID: 'solid'>,) failed: Error: Missing action function: setBeamType

session.wcs.beam.set_position(50, 50) ==> This applies to the "active" image only. On the GUI, we can configure the beam properties per image.

The WCSOverlay class is a fairly complicated one and I find it harder to follow the user manual (perhaps due to the structure of the code). For example,

From the user manual, it is not that straightforward to know that session.wcs.axes have the following user-facing attributes or methods 'color', 'custom_color', 'hide', 'set_color', 'set_custom_color', 'set_visible', 'set_width', 'show', 'visible', 'width' (I dump this with dir(session.wcs.axes). Would it be possible to make these options more explicit in the doc? Or should I refer to a different part of the doc to get such info?

[confluence]

I have fixed the names of the beam type property and action.

Regarding the inherited functions: if you click on the base class names listed under Bases, you can see the common functionality that is inherited from those classes. It is possible to show all inherited members in the Sphinx docs, but that adds a lot of noise, because all these objects inherit a lot of generic functionality from their ancestors (all the way up to call_action) -- it creates a lot of redundancy and makes it much harder to see the specific features of the classes, which is why it's not enabled. (You can see the effects for yourself if you add the :inherited-members: option to the module in carta.rst.)

So each class only shows the members defined in that class, and you have to click through to the parents explicitly. This is in line with what e.g. the casacore documentation does, so I don't think it's too unexpected, but we could add a general section to the docs explaining how to navigate the generated docs, and mention things like this. Alternatively, if you think it's a good idea, we could add a note to the docstring of each user-facing object (like the overlay components) that says something like This object inherits some common functionality from its base classes, or even ... from Foo, Bar and Baz (but the latter would be harder to maintain, and also redundant because there's already an automatically generated list of base classes just above).

[kswang1029]

Regarding session.wcs.colorbar.label few issue spotted:

1. missing colorbar.label.set_text() to customize the displayed label
2. doing colorbar.label.set_font(FontFamily.COURIER_NEW, FontStyle.BOLD) results in the label in italic style, not bold style
3. in colorbar.label.set_font() maybe the font size is missing?

[kswang1029]

Regarding session.wcs.colorbar.numbers, colorbar.numbers.custom_text results in

CartaBadResponse: CARTA scripting action .fetchParameter called with parameters (Macro('overlayStore.colorbar', 'numberCustomText'),) expected a response, but did not receive one.

In fact, I think we do not need colorbar.numbers.set_custom_text and colorbar.numbers.custom_text for numbers?

[confluence]

1. missing colorbar.label.set_text() to customize the displayed label

This is a value which is set per-image, like the image title. So you can disable and enable use of a custom label through the overlay API, but to set the text you use the set_custom_colorbar_label function on the image.

2. doing `colorbar.label.set_font(FontFamily.COURIER_NEW, FontStyle.BOLD)` results in the label in italic style, not bold style

It turns out that what I thought was a consistent ordering in the AST wrapper was not consistent (and I was using the one exception as the rule, which is why the result was incorrect for Courier New). I think this is fixed now (I adjusted the values of the style constants and added a fix for the exception).

3. in colorbar.label.set_font() maybe the font size is missing?

Yes, I completely forgot to add this to the common font functionality. It should now be available in all components with a font.

[confluence]

In fact, I think we do not need colorbar.numbers.set_custom_text and colorbar.numbers.custom_text for numbers?

Yes; I accidentally added this base class to a component which didn't have custom text.

[kswang1029]

@confluence apologies for the long wait. I have collected a set of comments while implementing e2e tests for the WCSOverlay class as well as some others. Please have a look.

session.wcs

1. session.wcs.beam # BUG: This only applies to active image. We need to be able to set beam properties per image.

2. assert border.color == PaletteColor.BLACK # BUG?: the rendered black is actually with rgba as (16,22,26,255), something to do with chrome settings? I am using "--force-color-profile=srgb". Should we apply this flag in the codebase as well so that colors rendered on different machine will be consistent?

3. missing colorbar.label.set_font_size()

4. missing colorbar.numbers.set_font_size()

5. colorbar.gradient.set_visible(True) # TODO: do we really need to expose this to users?

6. ticks.set_visible(False) # BUG: CartaActionFailed: CARTA scripting action overlayStore.ticks.setVisible called with parameters (False,) failed: Error: Missing action function: setVisible assert ticks.visible == False # BUG: CartaBadResponse: CARTA scripting action .fetchParameter called with parameters (Macro('overlayStore.ticks', 'visible'),) expected a response, but did not receive one. ticks.hide() # BUG: CartaActionFailed: CARTA scripting action overlayStore.ticks.setVisible called with parameters (False,) failed: Error: Missing action function: setVisible

   assert ticks.visible == False # BUG: as above

   ticks.show() # BUG: CartaActionFailed: CARTA scripting action overlayStore.ticks.setVisible called with parameters (True,) failed: Error: Missing action function: setVisible

7. session.wcs.toggle_labels() # TODO: do we need this? Need to know the state first before apply it to get what we need. It seems redundant to labels.set_visible(True/False)

8. missing control function wrappers in the Conversion tab when a pv image is loaded

9. missing title.set_text. title should be set to different images independently

10. missing pixel grid settings in session.wcs.grid

Image object BUG: cannot set center and channel via matched images

def raster_rendering():
    session = test_session_initializer()
    session.wcs.set_view_area(800, 400)

    imgs = session.open_images(["HD163296_CO_2_1_subimage.fits", 
                                "HD163296_13CO_2-1_subimage.fits"])
    imgs[1].set_cube_matching(True)
    imgs[1].set_raster_scaling_matching(True)
    imgs[0].raster.set_colormap("tab10")
    imgs[1].raster.set_colormap("tab10")
    imgs[0].set_channel(88)
    imgs[0].set_center(144, 117)
    imgs[0].zoom_to_size("8arcsec", "x")
    # BUG: cannot set center and channel via matched images
    #imgs[1].set_center(98, 98)
    #imgs[1].set_channel(60)
    session.save_rendered_view("raster_rendering.png")
    return "Done"

General

1. With my VSCode IDE, I seem not be able to see tips when hovering over the code. It would be great if we can show tips so that we do not need to consult the API user manual.

2. Do you think we can enforce typing (https://docs.python.org/3/library/typing.html) in the entire codebase?

[confluence]

@kswang1029 I'm going to have to dig into some of these in more detail, but here are my comments so far:

1. To be investigated.
2. Could you please provide more detailed steps to reproduce?
3. This should already have been fixed following feedback from @Jordatious -- please check that you have the latest code.
4. As above
5. The purpose of this is to allow users creating publication images to separate the gradient (which is a raster image) from the rest of the overlay (which is vectorisable line art). We will probably always need something like this to create a "perfect" image export in which vectors are vectors and rasters are rasters, so I'd rather not remove it preemptively.
6. I forgot that ticks are the only element with no visibility properties in AST. I have just removed this.
7. This is a shortcut for toggling the labels, numbers and title -- it's here because there's a button for it in the GUI. But we could remove it.
8. To be investigated.
9. This is not a global WCS setting. It's already a function on the image (image.set_custom_title), since it's a per-image operation.
10. To be investigated.

Image object bug: to be investigated.

General:

1. This is an issue with VSCode settings, which I don't know anything about. It should be possible to display docstrings as tips; possibly you need a language server like Pylance
2. This is possible, but it would be a major undertaking. It also has the tradeoff of adding complexity and slowing down future development. I considered this as an option early in development, but decided on a custom input validation system for user-facing high-level functions (which can check for more forms of input correctness than just type, such as allowed number ranges, allowed string patterns, discrete allowed options, etc.). This is certainly up for discussion, especially if more developers get involved, but I don't currently consider it a priority.

[kswang1029]

@confluence Regarding the color issue, I think it is due to the color profiles of the headless chrome (--force-color-profile=srgb). I spotted this issue a long while ago when comparing images from different computers and I saw although the contexts are identical, the rendered colors are not. Then I realized this is due to the color profile. Once I set the flag, I got exact the same result. You can give it a try by setting different color profiles to see the effect. As a result, in order to have identical result, I suggest we add this flag to the browser setup in the codebase.

[Jordatious]

Hi @confluence, I don't remember giving any feedback about colorbar.label.set_font_size()! I have tested this a little bit, but I've realised that to sufficiently test the new functionality will take quite some time, so I'm hoping you have it covered between yourselves!

However, I would like to ask whether it would be useful to invite one (or more!) of the AusSRC developers here to have a go at rigorously testing this. I think they will be quite good at that! For the last CARTA installation I did last week (v4.1), the scripting seemed to work well out of the box, and I didn't have to do anything special, like write and link to particular executables or anything. But perhaps some of the existing things I had in place, like "enable_scripting": true in ~/.carta/backend.json, needs to be configured. Are you able to clarify?

What I can say from my testing is that it's generally working really well, and I like the way the raster, contour and vector modules/classes are working. I think it's best having as many clear examples as possible (e.g. like the quick-start page), rather than relying on users reading the detailed docstring-style documentation! I did notice some strange behaviour with the rendering of vectors, but I wasn't able to spend long enough to see whether I was doing it correctly. I also found a bug with the following:

In [81]: img1.set_custom_colorbar_label?
Signature: img1.set_custom_colorbar_label(label_text)
Docstring:
Set a custom colorbar label for this image.

This also automatically enables custom colorbar label text for all images. It can be disabled with :obj:`carta.wcs_overlay.Colorbar.set_label_custom_text`.

Parameters
----------
label_text : a string
    The custom colorbar label text.
File:      ~/opt/anaconda3/envs/carta-python/lib/python3.12/site-packages/carta/image.py
Type:      method

In [82]: img1.set_custom_colorbar_label('Spectral Index')
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
Cell In[82], line 1
----> 1 img1.set_custom_colorbar_label('Spectral Index')

File ~/opt/anaconda3/envs/carta-python/lib/python3.12/site-packages/carta/validation.py:862, in validate.<locals>.decorator.<locals>.newfunc(self, *args, **kwargs)
    860     msg = STRIP_CODE.sub(r"\1", msg)
    861     raise CartaValidationFailed(f"Invalid function parameter passed to {func.__name__}: {msg}")
--> 862 return func(self, *args, **kwargs)

File ~/opt/anaconda3/envs/carta-python/lib/python3.12/site-packages/carta/image.py:274, in Image.set_custom_colorbar_label(self, label_text)
    264 """Set a custom colorbar label for this image.
    265
    266 This also automatically enables custom colorbar label text for all images. It can be disabled with :obj:`carta.wcs_overlay.Colorbar.set_label_custom_text`.
   (...)
    271     The custom colorbar label text.
    272 """
    273 self.call_action("setColorbarLabelCustomText", label_text)
--> 274 self.session.wcs.colorbar.set_label_custom_text(True)

AttributeError: 'Colorbar' object has no attribute 'set_label_custom_text'

[kswang1029]

@confluence adding a few more (missed from my test note🙈): 1) missing colorbar.label.set_text() 2) missing colorbar.label.text() 3) image.set_center()

def vector_field_rendering():
    session = test_session_initializer()
    session.wcs.set_view_area(800, 400)

    imgs = session.open_images(["HL_Tau.POLI.fits", 
                                "HL_Tau_StokesIQUV_clean.pbcor.fits"])
    imgs[1].set_spatial_matching(True)
    # likely a bug: AttributeError: 'Numbers' object has no attribute 'session'
    #imgs[1].set_center("4h31m38.429s", "18d13m57.20s")
    imgs[1].set_center(267, 252)
    imgs[1].zoom_to_size("1.88arcsec", "y")

[kswang1029]

@Jordatious do you have time to work on the tutorials based on the new API (once this PR is merged) as you suggested?

[confluence]

@Jordatious sorry; I was convinced that those were bugs you found, but it was actually @kswang1029 earlier!

For a local single-user CARTA installation (no controller), enabling scripting in the backend config is sufficient. For a controller installation, there is additionally a setting in the controller config (scriptingAccess needs to be set to enabled-all-users).

I will look into the bug you found.

@kswang1029 have you retested all of these issues with the latest version of the branch? Those sound a lot like other things I fixed earlier. Please also check the docs rebuilt from the latest version.

[confluence]

@Jordatious I just fixed that custom label text bug, and added a unit test for it (the unit tests for the image class aren't complete yet, which is why this wasn't picked up earlier).

[Jordatious]

Thanks @confluence! I've got someone specific in mind to help test this more extensively, who I'm meeting with on Friday. We also have a CARTA controller to which we should have complete admin access, so we should be able to test that as well! Would that be useful, and within the timeframes you're after here (e.g. feedback from next week onwards)?

@kswang1029, I think tutorials are a brilliant idea! I'll just have to see what time I can commit to this, depending on what it looks like exactly. However, I was rather referring to the quick-start documentation.

[confluence]

@Jordatious more testing is definitely useful; thank you! I think it would also be very valuable to have someone contribute to tutorials. The quick start page is just that -- an explanation of how to get started (with more detailed instructions for creating different kinds of sessions, but after that just a brief example of what you can do once you have the session). I think that a "cookbook" for performing different tasks is a great idea, but it belongs in the tutorials.

[Jordatious]

Excellent! And the timing works ok? i.e. you're not trying to get this out the door over the next week or so?

I agree, tutorials would be really helpful. I suppose I had thought @kswang1029 meant recorded demos, but perhaps you both mean documented cookbook examples.

[confluence]

@Jordatious I am definitely thinking of text-based cookbook examples -- videos are useful for GUIs, but I think they're a poor fit for scripts.

Regarding the timeline: we discussed this in a meeting, and will probably go ahead with merging this PR as soon as possible (to make it easier to test the regions PR and continue with the plan to split the branches, make a package, etc.), but we will welcome further feedback on the UI after that as new issues filed against the development branch (we can continue making incremental improvements). So there's no rush.

[Jordatious]

Thanks for the info @confluence. I spoke more with @d3v-null and they're keen to help contribute to this, which could include testing the new refactored and unified interface (i.e. this PR), the basic region support (PR #136), and perhaps additional general testing and contribution to some cookbook/tutorial examples (pending further discussion). We have our own AusSRC controller/server version that would be good to test, as well as our local desktop versions.

[confluence]

Noting here that this will implement #119 and #140.

[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 (#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 (#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.)

[confluence]

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