-
Notifications
You must be signed in to change notification settings - Fork 13
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
FEAT: Support sky regions and allow direct call to translator function #93
Conversation
direct call.
@@ -14,8 +14,7 @@ | |||
|
|||
__all__ = ["range_to_rect", "AstropyRegionsHandler"] | |||
|
|||
GLUE_LT_1_10 = Version(glue_version) < Version('1.10') | |||
GLUE_LT_1_10_1 = Version(glue_version) < Version('1.10.1.dev') # remove .dev after it is released | |||
GLUE_LT_1_11 = Version(glue_version) < Version('1.11') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhomeier , I am also cleaning up the version check here, as discussed in #92 (comment) . True annulus ROI support wasn't added till glue-core 1.11.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #93 +/- ##
==========================================
- Coverage 96.92% 96.89% -0.04%
==========================================
Files 18 18
Lines 1399 1415 +16
==========================================
+ Hits 1356 1371 +15
- Misses 43 44 +1
☔ View full report in Codecov by Sentry. |
This comment was marked as resolved.
This comment was marked as resolved.
sky region translation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems sensible to me - aside from a suggested API change. I'm ok with having the helper function be public API, but curious what the use case is compared to using the regular translator API in glue?
@@ -54,9 +53,54 @@ def range_to_rect(data, ori, low, high): | |||
return RectanglePixelRegion(PixCoord(xcen, ycen), width, height) | |||
|
|||
|
|||
def roi_subset_state_to_spatial(subset_state, to_sky=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
roi_subset_state_to_region
might make more sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what the use case is compared to using the regular translator API in glue?
I noticed that the translator code is basically being duplicated at https://github.com/spacetelescope/jdaviz/blob/8790525bcc31582b4828c4b0dfb50200621e3da6/jdaviz/app.py#L1054 . That code was added in for Jdaviz Subset Tools plugin to support display/edit of composite Subset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed it. Thanks for the review!
Description
There is a need for Jdaviz to be able to call the translator code directly and have option to get back sky regions. The first part is useful for spatial region translations where we pass in the subset state directly internally and there is really no need to access
subset.data
. This blocks:Fix #89
cc @dhomeier or @astrofrog