Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Basic region support #136

Open
wants to merge 50 commits into
base: dev
Choose a base branch
from
Open

Basic region support #136

wants to merge 50 commits into from

Conversation

confluence
Copy link
Collaborator

@confluence confluence commented Aug 16, 2023

This PR implements #80 and adds basic region support to the high-level wrapper. This PR depends on multiple frontend changes made in #2231, so that should be merged first.

Required before this can be moved out of draft:

  • Fill in missing region and annotation functions (lock, focus, style, modify dimensions, etc.)
  • Add support for WCS coordinates and sizes (requires retroactive change to some existing code for consistency)
  • Fill in missing docstrings
  • Add basic tests
  • Factor out BasePathMixin into a separate PR (it should be merged first and used to refactor the existing code to use the same composition pattern as the region functionality)
  • Manually inspect the generated documentation for bugs.

Outstanding issues to be addressed before merging

  • Make sure that the custom code that patches missing frontend features (size, etc.) is consistent with the dev frontend updates.

This PR should be tested against the latest release version of CARTA. There will be a separate PR to remove the custom code in response to the dev frontend fixes, once this is merged and a main branch is created.

Sorry, something went wrong.

@confluence confluence self-assigned this Aug 16, 2023
@confluence confluence marked this pull request as draft August 16, 2023 10:39
…terpreted as pixels. Removed option to set coordinate system in set_center; this must now be done separately. Updated docstrings and tests. Number validator no longer matches numeric strings.
@confluence
Copy link
Collaborator Author

Note to self: the behaviour of line-like region set_size should be updated to be consistent with CARTAvis/carta-frontend#2280.

(I.e. the wrapper behaviour with the last release version of CARTA should be consistent with the behaviour changes in that PR; it will be updated to work with the actual changes in the PR after this PR is merged and a main branch is created.)

@confluence confluence mentioned this pull request Dec 6, 2023
4 tasks
…ontend changes.
@confluence
Copy link
Collaborator Author

confluence commented Dec 7, 2023

The functionality of the line-like regions (getting / setting the size /rotation / center of the line region, line annotation, vector annotation, and ruler annotation) should now be the same in this wrapper + the release version of CARTA as it is in the development version of the frontend (once #2280 is merged, when tested with snippets). (A known difference is that for the ruler the rotation may sometimes be a float where the rotation for an identical line region is an integer -- but the values should be the same.)

@confluence
Copy link
Collaborator Author

@kswang1029 I wonder if we should adopt the elements nomenclature here as well.

@kswang1029
Copy link
Contributor

@kswang1029 I wonder if we should adopt the elements nomenclature here as well.

maybe not. Where in your mind should be?

@confluence
Copy link
Collaborator Author

maybe not. Where in your mind should be?

The wrapper is currently using "region" as a generic term for region or annotation, although in some places both are mentioned explicitly. I was wondering if we were moving away from that in the frontend, but I guess this change only affects one part of the UI (and e.g. we haven't renamed the region list). So what we have now is probably clear enough.

@kswang1029
Copy link
Contributor

maybe not. Where in your mind should be?

The wrapper is currently using "region" as a generic term for region or annotation, although in some places both are mentioned explicitly. I was wondering if we were moving away from that in the frontend, but I guess this change only affects one part of the UI (and e.g. we haven't renamed the region list). So what we have now is probably clear enough.

I see. Let’s keep things as they are now. Using “element” in the region file dialog is just to simply the info to one line. Otherwise we need two lines for region and annotation, respectively.

@Jordatious
Copy link

Hi @confluence, I'd like to have a little play with this. Do you have any documented examples or setting WCS regions, or could you provide some? I can get a vague idea from the docs and docstrings, but it would be useful to get started with some examples!

@confluence
Copy link
Collaborator Author

Hi, @Jordatious . I don't think there's currently any documentation for this outside the API docs. The general idea is:

  • You access the objects for existing regions with the list and get methods on your_image.regions
  • You create new regions with add_point, add_rectangle, and so on. The base add_region isn't intended for interactive use like this, but it's available in case you want to iterate over a whole bunch of region properties and add them programmatically. Unlike in the GUI, there's only one method for regions and annotations with the same shape (there's an annotation flag, which is false by default). These methods return the region object (the way that the session functions for opening images return an image object).
  • You can interact with existing regions using their objects -- you should be able to inspect or change any property. This includes changing the control points (the different region types have different helper methods for this, because the control points are interpreted differently). The docstrings should specify which control point parameters accept WCS coordinates / sizes or pixels, and which have to be one or the other. IIRC there's currently no way to change the type of a region, but if there's demand for this I could look into it (in the meantime, you can manually create a new region using properties taken from an existing region, and then remove the existing region).
  • You can import regions from a file with your_image.regions.import_from. You can export multiple regions with the corresponding export_to, and export a single region with the export_to on the region object.

I'm assuming that you're using the v4 release of CARTA to test, but just to make sure, please do that (the development version of the frontend has some new changes which will require updates to this code).

This branch doesn't include the general API changes from the other branch (yet, but I'm hoping to merge that branch in soon).

@confluence confluence linked an issue Mar 27, 2024 that may be closed by this pull request
@confluence
Copy link
Collaborator Author

The main refactoring PR has now been merged into dev, and dev has been merged into this PR.

@kswang1029
Copy link
Contributor

kswang1029 commented Aug 14, 2024

some comments for now:

  1. Region objects have some unnecessary attributes and methods. For examples:
  • point region: it has attributes and methods as

['CUSTOM_CLASS', 'REGION_TYPES', 'class', 'delattr', 'dict', 'dir', 'doc', 'eq', 'format', 'ge', 'getattribute', 'gt', 'hash', 'init', 'init_subclass', 'le', 'lt', 'module', 'ne', 'new', 'reduce', 'reduce_ex', 'repr', 'setattr', 'sizeof', 'str', 'subclasshook', 'weakref', '_base_path', '_region', 'call_action', 'center', 'color', 'control_points', 'dash_length', 'delete', 'existing', 'export_to', 'focus', 'from_list', 'get_value', 'line_width', 'lock', 'macro', 'name', 'new', 'point_shape', 'point_width', 'region_class', 'region_id', 'region_set', 'region_type', 'session', 'set_center', 'set_control_point', 'set_control_points', 'set_line_style', 'set_name', 'set_point_style', 'set_size', 'size', 'unlock', 'wcs_center', 'wcs_size']

line_width, set_size, etc are not required for a point region.

  • vector annotation: it has attributes and methods as

['CUSTOM_CLASS', 'REGION_TYPES', 'class', 'delattr', 'dict', 'dir', 'doc', 'eq', 'format', 'ge', 'getattribute', 'gt', 'hash', 'init', 'init_subclass', 'le', 'lt', 'module', 'ne', 'new', 'reduce', 'reduce_ex', 'repr', 'setattr', 'sizeof', 'str', 'subclasshook', 'weakref', '_base_path', '_region', 'call_action', 'center', 'color', 'control_points', 'dash_length', 'delete', 'endpoints', 'existing', 'export_to', 'focus', 'from_list', 'get_value', 'length', 'line_width', 'lock', 'macro', 'name', 'new', 'pointer_length', 'pointer_width', 'region_class', 'region_id', 'region_set', 'region_type', 'rotation', 'session', 'set_center', 'set_control_point', 'set_control_points', 'set_endpoints', 'set_length', 'set_line_style', 'set_name', 'set_pointer_style', 'set_rotation', 'set_size', 'size', 'unlock', 'wcs_center', 'wcs_endpoints', 'wcs_length', 'wcs_size']

set_size, size etc are not required for a vector annotation.

There are other examples in different regions and annotations. Based on the GUI, I compiled a table for the props of regions/annotations: https://docs.google.com/spreadsheets/d/1XMYA2g1QqzWdm-AQCUlamCKGCiF5819D7ujM5Jv95X0/edit?usp=sharing From there we can see common props are only a few so maybe the base class should only include those. Also from the table, we can see there are groups with common props (*: with additional props):

  • point
    • point region
    • point annotation
  • line
    • line region
    • line annotation
    • vector annotation*
  • rectangle
    • rectangle region
    • rectangle annotation
    • text annotation*
  • ellipse
    • ellipse region
    • ellipse annotation
  • polygon
    • polygon region
    • polygon annotation
    • polyline region
    • polyline annotation
  • misc
    • compass annotation (~vector annotation + text annotation)
    • ruler annotation (~line annotation + text annotation)

So my question is, can we have relevant props per region/annotation object (cf GUI)? We may need to reorganize the region class hierarchy?

  1. Dual methods for creating some types of regions/annotations

    add_rectangle(center, size, annotation=False, rotation=0, name='')
    - in this case, can we also support the definition with the two diagonal corners (like what we can do via GUI)?
    - in this case (and ellipse), I suggest we arrange the positional arguments as
    add_rectangle(center, size, rotation=0, annotation=False, name='')

    add_vector(start, end, name='')
    - in this case, can we also support the definition with a center, a length, and a position angle?

  2. in the GUI we use P.A. as the "rotation" angle for some regions. Shall we consider make it consistent in the two interfaces? In fact, shall we try to make the names of props consistent (eg (py) pointer_length vs (GUI) arrowhead length) in the two interfaces?

@confluence
Copy link
Collaborator Author

@kswang1029 I'm going to go through your comments in more detail, but briefly:

  • My intention was to provide relevant methods to similar objects, but also a consistent interface between different types -- so that, for example, size / set_size do something sensible for any object (even if this option is not available in the GUI). So the lack of 1:1 correspondence with the GUI is intentional -- but perhaps some of my choices are confusing (I agree that having a size for point regions is weird -- but does the line width not change the appearance of the point? I will test this). I will provide a more detailed response.
  • I treated the ruler as a line-like object, and tried to make its interface consistent with the other line objects.
  • It should be easy to add the option to create rectangles and ellipses using corners; I'll look into this.
  • I agree that the property names should be consistent; in some cases I followed the names in the frontend source (which don't always match the UI). I will check these.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support regions/annotations
3 participants