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

[ENH] ConcurrentWidgetMixin: Cancel task on input change #4219

Merged
merged 2 commits into from
Dec 16, 2019

Conversation

VesnaT
Copy link
Contributor

@VesnaT VesnaT commented Nov 25, 2019

Issue

ConcurrentWidgetMixin subclasses don't stop computation when an input changes.

Description of changes

Set invalidated widget state (instead of blocking state) when stopping/starting the task.

Includes
  • Code changes
  • Tests
  • Documentation

@VesnaT VesnaT force-pushed the cancel_task_on_new_input branch from ee78f65 to 1bff0cd Compare November 25, 2019 08:43
@codecov
Copy link

codecov bot commented Nov 25, 2019

Codecov Report

Merging #4219 into master will decrease coverage by 0.02%.
The diff coverage is 95.83%.

@@            Coverage Diff            @@
##           master   #4219      +/-   ##
=========================================
- Coverage   86.23%   86.2%   -0.03%     
=========================================
  Files         396     396              
  Lines       70643   70635       -8     
=========================================
- Hits        60918   60890      -28     
- Misses       9725    9745      +20

@ales-erjavec
Copy link
Contributor

If i connect File -> t-SNE, and load iris.tab then housing.tab, then move focus (tab) to the 'Recent' combo box and spam the down arrow key, eventually I get a:

Traceback (most recent call last):
  File "/Users/aleserjavec/workspace/orange3/Orange/widgets/unsupervised/owtsne.py", line 551, in on_partial_result
    self.__ensure_task_same_for_pca(task)
  File "/Users/aleserjavec/workspace/orange3/Orange/widgets/unsupervised/owtsne.py", line 516, in __ensure_task_same_for_pca
    assert task.data is self.data
AssertionError

@VesnaT VesnaT changed the title [ENH] ConcurrentWidgetMixin: Cancel task on input change [WIP][ENH] ConcurrentWidgetMixin: Cancel task on input change Nov 29, 2019
@VesnaT VesnaT changed the title [WIP][ENH] ConcurrentWidgetMixin: Cancel task on input change [ENH] ConcurrentWidgetMixin: Cancel task on input change Dec 5, 2019
@janezd
Copy link
Contributor

janezd commented Dec 5, 2019

Please rebase to current master.

@VesnaT VesnaT force-pushed the cancel_task_on_new_input branch from 0957f5e to 84cb23e Compare December 6, 2019 10:12
@@ -513,7 +513,7 @@ def run(self):

def __ensure_task_same_for_pca(self, task: Task):
assert self.data is not None
assert task.data is self.data
np.testing.assert_array_equal(task.data.X, self.data.X)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

numpy.testing should not be used outside of unittests.

I am assuming the cause of this assert violation is a check that the data is the same upon receiving the input but only after having already updated self.data.

If that is the case I would just remove the assert.

Copy link
Contributor Author

@VesnaT VesnaT Dec 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's replaced with (task.data.X == self.data.X).all()

EDIT: I'll remove it.

@VesnaT VesnaT force-pushed the cancel_task_on_new_input branch 3 times, most recently from 50a8a21 to 770ba0e Compare December 13, 2019 10:58
The data instance could have changed, but the task was not canceled due to self._invalidated = False.
@VesnaT VesnaT force-pushed the cancel_task_on_new_input branch from 770ba0e to 50b7f96 Compare December 13, 2019 11:36
@ales-erjavec ales-erjavec merged commit a20b32f into biolab:master Dec 16, 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.

3 participants