-
Notifications
You must be signed in to change notification settings - Fork 0
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
base: dev
Are you sure you want to change the base?
Basic region support #136
Conversation
…lly by setup and docs; fixed typo in import
…ng a region and clearing all regions.
…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.
…ses. Refactored region class hierarchy.
…d inconsistent line region size functions
Note to self: the behaviour of line-like region (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.) |
…ontend changes.
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.) |
@kswang1029 I wonder if we should adopt the |
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. |
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! |
Hi, @Jordatious . I don't think there's currently any documentation for this outside the API docs. The general idea is:
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). |
The main refactoring PR has now been merged into dev, and dev has been merged into this PR. |
some comments for now:
line_width, set_size, etc are not required for a point region.
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):
So my question is, can we have relevant props per region/annotation object (cf GUI)? We may need to reorganize the region class hierarchy?
|
@kswang1029 I'm going to go through your comments in more detail, but briefly:
|
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:
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)Outstanding issues to be addressed before merging
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.