Rampviz fixes for unexpected viewer limit resets #3171
Conversation
bmorris3
requested review from
rosteen,
javerbukh,
pllim,
kecnry,
haticekaratay,
cshanahan1 and
gibsongreen
as code owners
github-actions
bot
added
plugin
bmorris3
force-pushed
the
rampviz-slider-reset-limits
branch
from
b6c865c to
2501cd5
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3171 +/- ##
=======================================
Coverage 88.42% 88.42%
=======================================
Files 122 122
Lines 18275 18286 +11
=======================================
+ Hits 16159 16169 +10
- Misses 2116 2117 +1 ☔ View full report in Codecov by Sentry. |
| @@ -198,7 +198,11 @@ def _expected_subset_layer_default(self, layer_state): | |||
| return | |||
|
|
|||
| # default visibility based on the visibility of the "parent" data layer | |||
| layer_state.visible = self._get_layer(layer_state.layer.data.label).visible | |||
| if self.__class__.__name__ != 'RampvizProfileView': | |||
There was a problem hiding this comment.
can this not do an isinstance check because of circular import?
| else: | ||
| # Rampviz doesn't show subset profiles by default: | ||
| layer_state.visible = False |
There was a problem hiding this comment.
this else applies to more than just rampviz though, right?
There was a problem hiding this comment.
I'm not sure what you mean - the logic introduced here only affects Rampviz.
There was a problem hiding this comment.
yes, but this is in the general JdavizViewerMixin used by viewers across all configs, so the else will be entered by all other cases than RampvizProfileView. For non-rampviz cases, we need the previous behavior of adopting visibility from the corresponding parent data layer.
There was a problem hiding this comment.
The above if is:
if self.__class__.__name__ != 'RampvizProfileView':There was a problem hiding this comment.
ah, I misread that as == 🤦 and further confused because the comment in the else mentions rampviz. Similar confusion might be avoided by switching the if and else so the else is the default case (or just paying more attention on my part).
There was a problem hiding this comment.
both are true. I'll switch :D
bmorris3
force-pushed
the
rampviz-slider-reset-limits
branch
from
1bc8208 to
d2aef1b
Compare
bmorris3
force-pushed
the
rampviz-slider-reset-limits
branch
from
d2aef1b to
64b4a75
Compare
Description
Three bugs to fix:
When updating the selected slice in the integration viewer in Rampviz, the integration viewer's y-limits change twice and the data flash and move around a bit.
When a subset is created in the group-viewer, the integration viewer gets a subset profile by default via glue. That profile layer's icon appears in the legend and
layer.visible == True, even though the subset profile can't be seen in the viewer. Glue ensures that profile viewers create visible subset profiles by default, it's on us to override that behavior, and some minor bits of logic are missing.When using the single-pixel ramp-extraction-on-hover tool, integration-viewer limits do not get reset when the preview exceeds the y-limits.
This PR fixes these bugs by:
integration_viewer.reset_limitsto avoid redundant callsChange log entry
CHANGES.rst? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rstbefore merge. If no, maintainershould add a
no-changelog-entry-neededlabel.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.
triviallabel.