-
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
auto-update support for plugin results #2680
Conversation
jdaviz/core/template_mixin.py
Outdated
for registry_name, tray_item in tray_registry.members.items(): | ||
if tray_item['cls'] == self.__class__: | ||
# If viewer reference names need to be passed to the tray item | ||
# constructor, pass the names into the constructor in the format | ||
# that the tray items expect. | ||
tray_registry_options = tray_item.get('viewer_reference_name_kwargs', {}) | ||
for opt_attr, [opt_kwarg, get_name_kwargs] in tray_registry_options.items(): | ||
opt_value = getattr( | ||
self, opt_attr, app._get_first_viewer_reference_name(**get_name_kwargs) | ||
) | ||
|
||
if opt_value is None: | ||
continue |
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 for-loop is ugly. I have a local stash that attempts to replace this by having the registry itself set cls._viewer_reference_name_kwargs
which shows promise but still needs some work.
bf06f44
to
c926cc4
Compare
3dba0ab
to
325792c
Compare
f5de0bf
to
d2035db
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2680 +/- ##
==========================================
+ Coverage 88.82% 88.88% +0.06%
==========================================
Files 111 111
Lines 16879 16980 +101
==========================================
+ Hits 14993 15093 +100
- Misses 1886 1887 +1 ☔ View full report in Codecov by Sentry. |
59b8a48
to
a5350d8
Compare
There is some flashing that occurs when updating a subset and I do not see the second spatial subset subregion render in the viewer when creating a composite subset. Otherwise, this is looking promising! Are there other features you would like tested that are not mentioned in the description above? |
Can you explain what you mean here? Is that the auto-collapsed spectrum you're referring to - and if so, does that happen before this PR? The plan is to use this to replace that functionality in the near future, so if that is the case, it doesn't worry me too much, but would be weird if it was introduced by this PR. |
2fac50c
to
dbab3d3
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.
I tried this out and it works, with one exception that I noticed.
If you create a composite subset, the sum is automatically plotted in the spectrum viewer upon creation (by glue, not going through the plugin). However, the spectral extraction doesn't support composite subsets, so the auto-update fails with a non-informative traceback, and you just can't create the composite subset. I think this can be addressed with a try-except (that would also catch other failures) that disables the auto-update but still allows you to modify the subset. As a follow up, but not now since this issue existed before this PR, be more clear in the plugin that composite subsets aren't valid selections (until that functionality is enabled).
* move logic for assigning default viewer references from the app-method to the plugin itself, by searching the registry for a match * create "new" convenience method on plugin * allows for running results from a saved plugin state without altering the user-facing instance of the plugin
rather than relying on async callback with sleep
0060495
to
4677435
Compare
* changed to be the human-friendly name in spacetelescope#2680, but missed updating a few cases, resulting in moment maps not being linked correctly
* fix reference to plugins in metadata * changed to be the human-friendly name in #2680, but missed updating a few cases, resulting in moment maps not being linked correctly * TST: Add link cids check as regression test. This check currently fails on main but should be passing as part of PR 2826 --------- Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>
Description
This pull request implements the infrastructure for plugin results to know their inputs and update when the input data and/or subset is changed. Currently this is only implemented for the spectral extraction (in cubeviz) plugin, but is generalized so that it could be extended to other plugins as well.
Screen.Recording.2024-01-30.at.9.27.10.AM.mov
Remaining work to generalize if continuing in this direction:
__call__
or similar method?could store the hash of the input subset(s)/dataset(s) to avoid reacting to duplicate messages (subset update message from multiple viewers, etc), or filter and only listen to subset updates from the default viewer.data
andspectral_subset
, but this needs to be generalized to listen to any arbitrary data or subset inputs.hide UI behind feature flag until replacing of collapse is in place? (may not be necessary now that we're intentionally deferring this until after the 3.9 release)Potential follow-ups:
_set_data_component
).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.