Preserve plot options when replacing data from a plugin #2288
Conversation
rosteen
requested review from
duytnguyendtn,
javerbukh,
pllim,
kecnry,
bmorris3,
haticekaratay and
cshanahan1
as code owners
|
I am out of time for this sprint, so I removed myself from the review pool. FYI. |
Update test so that it actually fails on main
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #2288 +/- ##
==========================================
+ Coverage 90.89% 90.96% +0.07%
==========================================
Files 152 152
Lines 17060 17173 +113
==========================================
+ Hits 15506 15622 +116
+ Misses 1554 1551 -3
☔ View full report in Codecov by Sentry. |
| add_to_viewer_refs.append(viewer_ref) | ||
| add_to_viewer_vis.append(label in viewer_item['visible_layers']) |
There was a problem hiding this comment.
couldn't this result in duplicating the entry in the list if there are multiple layers? (I'm trying to think of what the consequences would be to test...)
There was a problem hiding this comment.
Hmm, perhaps, you're right that this logic isn't perfectly equivalent to the previous one. How about just adding a break at the end of this else clause?
There was a problem hiding this comment.
I guess since you're only acting on the current viewer, only one layer should ever enter the else, so its probably fine (which explains why I couldn't find a way to break it in testing). It might be more obvious for future us though to do something like
layer = next((layer for layer in viewer.layers if layer.layer.label == label), None)
# everything that is currently in the else
(or maybe we should implement viewer.get_layer(label)?)
jdaviz/core/template_mixin.py
Outdated
|
|
||
| # Note that we can only preserve one of percentile or vmin+vmax | ||
| preserve_attributes = ("color", "alpha", "bias", "linewidth", "stretch", | ||
| "v_min", "v_max", "cmap") | ||
|
|
There was a problem hiding this comment.
Is there any way to access these programmatically since different layer-types might have different options (thinking for image layers created in cubeviz, or scatter layers in lcviz where keeping this list up-to-date with upstream changes might be difficult and if we are ever short any entries, that could be quite confusing to the user). Maybe using layer.state.as_dict() and just hardcoding certain attributes to ignore instead (like the link to layer.state.layer)?
There was a problem hiding this comment.
There are enough attributes in state that I think listing the ones to ignore would be more annoying than listing the ones we want. I do see the annoyance with needing to potentially update this (and I didn't think of scatter viewers in this list), but I don't see a good programmatic way to do it that is less annoying.
There was a problem hiding this comment.
as_dict contains a lot less entries than dir(layer.state) though... we might only need to exclude ['attribute', 'layer']
There was a problem hiding this comment.
It's still a pretty big list, and there's some stuff in there that I'm either not sure what it does and/or wasn't sure if we want to preserve it, e.g. c_min, c_max, levels - are these all contour things? And if so, do we want to preserve contour levels? I rate that as a solid maybe. Maybe it is alright to default to keeping everything but the two you mentioned though.
There was a problem hiding this comment.
Preserving contours might be a PO question.
There was a problem hiding this comment.
ok, agreed, let's ask POs (cc @camipacifici). If it is a fixed list, then as-is works with me. If the default should be everything except a few, then I think it makes more sense to hardcode the exceptions than the inclusions.
There was a problem hiding this comment.
I think you can safely preserve everything (including contour levels).
I do not know why 'attribute' and 'layer' should not be preserved, but I trust you all on this.
|
I have successfully run tests locally and manually tested the desired behavior. Everything looks good to me. Thanks! |
|
@haticekaratay , do you want to officially approve? Thanks! |
|
@kecnry I switched to an ignore list for the attributes instead of an include list. |
|
Hmm, test failure looks real but I'm not sure why it's failing in 3.10 and not 3.9. |
|
It is failing because of remote data test that is triggered only on that job, not because of Python version. FYI. |
|
Remaining (new) test failure is unrelated, due to an upstream change. Merging. |
|
Oh, and the codecov report seems to have picked up lines that weren't new in this PR for some reason. |
This preserves a list of plot options when data is replaced from a plugin, for example when running model fitting a second time with the same output label. There was a decision to be made on whether to preserve
percentileorvminandvmaxfor images - I decided on the latter since it will be easier to see differences from the prior data.Opening as draft since I need to add tests still.