Skip to content
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

[FIX] OWProjectionWidgetBase: Update when domain is changed #3946

Merged
merged 1 commit into from
Aug 7, 2019

Conversation

janezd
Copy link
Contributor

@janezd janezd commented Jul 27, 2019

Fixes #3944.

@VesnaT, please confirm this is the right way to do it and that nothing else is needed - also considering possible other similar bugs.

@janezd janezd changed the title OWProjectionWidgetBase: Update when domain is changed [FIX]OWProjectionWidgetBase: Update when domain is changed Jul 27, 2019
@janezd janezd changed the title [FIX]OWProjectionWidgetBase: Update when domain is changed [FIX] OWProjectionWidgetBase: Update when domain is changed Jul 27, 2019
@janezd janezd force-pushed the scatterplot-changed-domain branch from e2aaa95 to 5d36adf Compare July 31, 2019 15:39
@codecov
Copy link

codecov bot commented Jul 31, 2019

Codecov Report

Merging #3946 into master will decrease coverage by 0.08%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3946      +/-   ##
==========================================
- Coverage   85.21%   85.13%   -0.09%     
==========================================
  Files         382      378       -4     
  Lines       67590    67117     -473     
==========================================
- Hits        57600    57137     -463     
+ Misses       9990     9980      -10

@VesnaT VesnaT self-assigned this Aug 2, 2019
@VesnaT
Copy link
Contributor

VesnaT commented Aug 2, 2019

No, this is not the right way to solve the issue. Setting _invalidated to True, causes recomputation of the embedding (in this - Scatterplot's - case there is no calculation and the replotting is not as wasteful as in other widgets that inherit from OWDataProjectionWidget, but should still be avoided).

  1. One of the solutions to the issue, could be to call OWScatterPlotGraph.update_axes in OWScatterPlot.handleNewSignals (not the best one because the axes are always updated):
super().handleNewSignals()
self.graph.update_axes()
self._vizrank_color_change()
  1. Another one (probably the best one) is to introduce a new flag self._domain_invalidated in OWDataProjectionWidget and call the OWScatterPlotGraph.update_axes only if it is set to True.
self._domain_invalidated = not (
                data_existed and self.data is not None and
                effective_data.domain.checksum()
                == self.effective_data.domain.checksum())
  1. And if you want a general solution (even though I'd rather not plot data again if not changed):
def handleNewSignals(self):
       self._handle_subset_data()
       if self._invalidated:
           self._invalidated = False
           self.setup_plot()
       else:
           self.graph.update_coordinates()
           self.graph.update_point_props()
       self.commit()

@VesnaT VesnaT removed their assignment Aug 2, 2019
@janezd janezd force-pushed the scatterplot-changed-domain branch from 5d36adf to 80c0cbb Compare August 4, 2019 17:40
@janezd
Copy link
Contributor Author

janezd commented Aug 4, 2019

My reasoning was that renaming variables is so uncommon that we can afford to update the entire graph. Still, your solutions are better. I would prefer the first one; updating axes costs nothing and simplicity counts. But I implemented the second because other widgets may also need to know that domain has changed.

@VesnaT VesnaT merged commit 530256a into biolab:master Aug 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scatter Plot axes labels not updated automatically
2 participants