-
Notifications
You must be signed in to change notification settings - Fork 74
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
Create import_region method for loading subsets #3082
base: main
Are you sure you want to change the base?
Conversation
2bffb52
to
73c99ad
Compare
73c99ad
to
d0242cf
Compare
test_subset_dict = {'Subset 1': [ | ||
{'name': 'TrueCircularROI', | ||
'glue_state': 'AndState', | ||
'region': CirclePixelRegion(center=PixCoord(x=20.6072998046875, y=23.749065399169922), radius=4.079286151278358), | ||
'sky_region': None, | ||
'subset_state': None}, | ||
{'name': 'RectangularROI', | ||
'glue_state': 'OrState', | ||
'region': RectanglePixelRegion(center=PixCoord(x=20.68303102318066, y=30.615620480255785), width=6.815691577924664, height=4.272523078699049, angle=0.0 * u.rad), | ||
'sky_region': None, | ||
'subset_state': None}, | ||
{'name': 'EllipticalROI', | ||
'glue_state': 'AndNotState', | ||
'region': EllipsePixelRegion(center=PixCoord(x=20.30438232421875, y=27.716407775878906), width=3.427145004272461, height=8.200210571289062, angle=0.0 * u.rad), | ||
'sky_region': None, | ||
'subset_state': None}], | ||
'Subset 2': [ | ||
{'name': 'CircularAnnulusROI', | ||
'glue_state': 'RoiSubsetState', | ||
'region': CircleAnnulusPixelRegion(center=PixCoord(x=30.452190399169922, y=22.070573806762695), inner_radius=2.4145185947418213, outer_radius=4.829037189483643), | ||
'sky_region': None, | ||
'subset_state': None}], | ||
'Subset 3': SpectralRegion(6.939254409861322 * u.um, 7.224590119773996 * u.um) | ||
} | ||
|
||
test_subset_string = str(test_subset_dict) | ||
|
||
region = [{'name': 'TrueCircularROI', | ||
'glue_state': 'RoiSubsetState', | ||
'region': CirclePixelRegion(center=PixCoord(x=25.888399124145508, y=22.078184127807617), | ||
radius=3.7648651881914112), | ||
'sky_region': None, | ||
'subset_state': ''}, | ||
{'name': 'TrueCircularROI', | ||
'glue_state': 'OrState', | ||
'region': CirclePixelRegion(center=PixCoord(x=19.990379333496094, y=30.782743453979492), | ||
radius=2.756227940196304), | ||
'sky_region': None, | ||
'subset_state': ''}] |
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.
aren't these internal formats? Are we expecting users to write and pass regions in this format or do we need support for this internally?
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 is the format that app.get_subsets
takes and it provides all relevant information for reconstructing subsets, so I thought it made sense to use as the format for importing as well.
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 thought we had planned to just support file or regions objects import, and leave more complex logic to importing successively and using the plugins API to change the combination mode?
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 can add support for that as well. My plan is to use the import_region
method to support any kind of import, SpectralRegion, Region object, dictionary, list, and string. I can work on the plugin api next.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3082 +/- ##
==========================================
- Coverage 88.76% 88.73% -0.03%
==========================================
Files 111 111
Lines 17246 17281 +35
==========================================
+ Hits 15308 15334 +26
- Misses 1938 1947 +9 ☔ View full report in Codecov by Sentry. |
Description
This pull request adds an
import_region
method which will be able to load subsets in the following formats:Change log entry
CHANGES.rst
? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rst
before merge. If no, maintainershould add a
no-changelog-entry-needed
label.Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
trivial
label.