-
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
FEAT: Load annulus from file, load reg files from IMPORT DATA #2201
Conversation
and allow IMPORT DATA to load reg files. Annulus support provided by glue-viz/glue#2403 and glue-viz/glue-astronomy#92 Add test for exception. Bump minimum requirements for glue-core and glue-astronomy.
1e679fa
to
beb45e0
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #2201 +/- ##
=======================================
Coverage 91.62% 91.63%
=======================================
Files 148 148
Lines 16535 16548 +13
=======================================
+ Hits 15151 15163 +12
- Misses 1384 1385 +1
☔ View full report in Codecov by Sentry. |
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.
Looks good! Only question is do we want to keep using get_interactive_regions()
over get_subsets()
? I guess if we move away from using subsets in imviz the answer is "yes", but if we go another way it might be something to consider (out-of-scope for this PR though).
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.
Just one quick question, but otherwise the diff looks reasonable to me and the demo seems to work as expected!
If this doesn't currently catch the exception and show in the load file dialog, that would definitely be nice to include, if possible (like other errors where a wrong file extension is selected, etc).
@@ -12,6 +12,11 @@ Cubeviz | |||
Imviz | |||
^^^^^ | |||
|
|||
- Added the ability to load DS9 region files (``.reg``) using the ``IMPORT DATA`` | |||
button. However, this only works after loading at least one image into Imviz. [#2201] |
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 happens if you try the .reg
file before loading an image? Is the ValueError
shown in the UI or as a traceback in the console?
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.
It throws error and this is tested on L188 in test_regions.py
in this PR:
with pytest.raises(ValueError, match="Cannot load regions without data"):
imviz_helper.load_data(self.region_file)
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.
The error would bubble up in the same was as any other error in Imviz parsing.
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.
Good question and @kecnry had mentioned before in a few other occasions. I also agree it is out of scope here. Kyle, is there a ticket? |
Since this has 2 approvals, I am going to merge. If you don't like how the loading error is presented, it also affects other parsing logic route and should be addressed separately. Thanks for the reviews! |
Description
This pull request is to add two new features:
Blocked by
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.