-
Notifications
You must be signed in to change notification settings - Fork 79
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Extract on SubsetCreateMessage #3238
base: main
Are you sure you want to change the base?
Conversation
@@ -262,6 +262,7 @@ def _aperture_items_changed(self, msg): | |||
self._extract_in_new_instance(subset_lbl=subset_lbl, | |||
auto_update=True, add_data=True) | |||
except Exception: | |||
raise |
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 here for debugging to expose the error.
self.hub.subscribe(self, SubsetCreateMessage, | ||
handler=lambda msg: self._update_subset(msg.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 hoped that I could just replace the trigger for this from SubsetUpdateMessage
to SubsetCreateMessage
, but it didn't work.
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.
Probably cannot work as a drop-in replacement since SubsetCreateMessage
does not have an attribute
(in fact, does nothing). Wondering if this would need something equivalent of LayerSelect._on_subset_created
, or if _update_subset
can tricked into doing the right action some other way.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3238 +/- ##
==========================================
- Coverage 87.62% 87.52% -0.10%
==========================================
Files 128 128
Lines 19844 19902 +58
==========================================
+ Hits 17388 17419 +31
- Misses 2456 2483 +27 ☔ View full report in Codecov by Sentry. |
subset_id=aperture.selected, cls=StdDevUncertainty | ||
) | ||
except ValueError: | ||
raise |
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.
raise | |
msg = SnackbarMessage( | |
f"Extracting uncertainties on subset for {aperture.selected} failed", # noqa | |
color='error', sender=self, timeout=10000) | |
self.app.hub.broadcast(msg) |
This would at least let the subset propagate to the spectral extraction tool, even if uncertainties are (initially) missing. I'd expect them to be available after the first modification (move/resize) to the subset, since I don't see the snackbar message repeated then, but could not find anything where nddate.uncertainty
is used.
2a41bed
to
c48aa92
Compare
c48aa92
to
6b0fa22
Compare
bb41b05
to
c3ef0b1
Compare
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.
Well code looks simple enough - glad to see the diff is so small! Just need to do some thorough testing and see if there are any existing tickets we can close out with this and/or a bump of glue.
Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>
creating a spectral subset in specviz does not show the entry in the legend/data-menu with glue-dev (but does when reverting glue before the related upstream PR). |
You're right, I had tested spectral subsets in Cubeviz but apparently it's behaving differently in Specviz. Creating another subset triggers the previous one to show up, I remember encountering this before but thought it was resolved 🤔 |
It seems like the layer hasn't been created yet when we're processing the SubsetCreateMessage in the data menu, but it's there when Plot Options is handling the message:
|
This is meant to be a compatibility patch for glue-viz/glue#2515, but currently isn't working. Opening this PR to share with SMEs @astrofrog @dhomeier. To reproduce the error I mentioned in here, open Cubeviz and try to use the Spectral Extraction plugin.