-
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
Implement associations between Data layers #2761
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2761 +/- ##
==========================================
- Coverage 88.68% 88.67% -0.01%
==========================================
Files 108 108
Lines 16214 16266 +52
==========================================
+ Hits 14379 14424 +45
- Misses 1835 1842 +7 ☔ View full report in Codecov by Sentry. |
@kecnry Suggested that we prevent assigning children of children for now. This has been added in
|
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.
There are some future decisions about whether these layers remain in the data-menu/legend/mouseover or just appear as part of the parent layer to the user, but since this isn't being advertised to the user yet as an option to load_data
, we can defer those decisions until the rest of the effort converges.
I do wonder whether we're building this with more flexibility than we'll ever need, but there are safeguards now to prevent nesting, etc, and at least if we want the flexibility later we won't have to redesign the whole infrastructure.
I just have a few minor comments on the implementation, otherwise am ok with this going in and iterating as needed for the follow-up efforts.
jdaviz/app.py
Outdated
for i, child_layer in enumerate(children_layers, start=1): | ||
self.state.layer_icons = { | ||
**self.state.layer_icons, | ||
child_layer: f'{parent_icon}{i}' | ||
} |
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 will update the state a bunch of times. Can we instead either build this logic into above or use a dictionary-comprehension to build all the new entries and then just apply to the state once?
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 idea. In 00785d7, I instead gather the child layer icon updates in a different dict, and update self.state.layer_icons
once at the end.
jdaviz/app.py
Outdated
if parent is not None: | ||
# Does the parent Data have a parent? If so, raise error: | ||
parent_of_parent = self._get_assoc_data_parent(parent) | ||
if parent_of_parent is not None: | ||
raise NotImplementedError('Data associations are currently supported ' | ||
'between root layers (without parents) and their ' | ||
f'children, but the proposed parent "{parent}" has ' | ||
f'parent "{parent_of_parent}".') | ||
self._set_assoc_data_as_child(data_label, new_parent_label=parent) |
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.
do we also need to validate that parent
exists in the data-collection?
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.
Sure thing, done in 55d2e68.
Adding a note for posterity: the above example calls imviz.load_data(path, ext='DQ', parent=parent) so it might look like the new keyword argument |
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 demo was exciting, can't wait to see how DQ progresses forward!
Thanks all! |
Description
This PR adds a dictionary
_data_associations
to theApplication
class, and methods for finding associations between glue Data entries in theDataCollection
.Suppose you have two layers A and B which are associated with one another, e.g., layer B is the DQ array for the science data in layer A. You could load these layers and specify their association like so:
which gets represented as a dictionary:
Four simple, private helper methods are added to
Application
to make this easy to use:_add_assoc_data_as_parent
: create an entry in the association dict with this layer as parent (called by default onadd_data
)_set_assoc_data_as_child
: specify that this layer is the child of another_get_assoc_data_parent
/_get_assoc_data_children
: find the parent/children of this layerFor now, I'm assuming:
It follows that we need to filter out children Data entries from
DatasetSelect
andLayerSelect
. This prevents the DQ layers from appearing in Plot Options or plugin data dropdown menus so they are not options in, e.g., Aperture Photometry.We could choose to expose the child layers in the data menu and/or legend, or hide them in either or both places. For now, I updated the layer icon logic to provide a special icon for child layers. In the above example, the layer labeled "B" will appear in the legend in Imviz as "A1" (for now).
Other uses to investigate in the future
ext='CON'
), which are represented as integer arrays with the same shape as the science data.Example
Here's an example using glue dev, since merge of glue-viz/glue#2468. It loads two JWST Level 2 images from the FRESCO program and their data quality extensions.
imviz-data-assoc.mov
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.