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] DendrogramWidget: Remove event filters before removing items #4361

Merged
merged 2 commits into from
Jan 24, 2020

Conversation

ales-erjavec
Copy link
Contributor

Issue

Fix what appears to be a quadratic time bottleneck in DendrogramWidget.clear

Alleviates gh-2663

Description of changes
  • Remove event filters before removing items
    Each removeItem seems to be linear in number of installed filters in the whole scene -> so this would be quadratic for us (not accounting any other filters not part of DendrogramWidget).
Includes
  • Code changes
  • Tests
  • Documentation

Each removeItem is linear in number of installed filters in the whole
scene -> so this would be quadratic for us (not accounting for other
filters not part of DendrogramWidget).
@ales-erjavec
Copy link
Contributor Author

Benchmarked using:

import timeit

import numpy as np
from scipy.cluster.hierarchy import linkage

from PyQt5.QtWidgets import QApplication, QGraphicsScene, QGraphicsView
from Orange.widgets.unsupervised.owhierarchicalclustering import DendrogramWidget
from Orange.clustering import hierarchical as hier

app = QApplication([])
scene = QGraphicsScene()
view = QGraphicsView()
view.setScene(scene)

N = 15000

dw = DendrogramWidget()
scene.addItem(dw)

mat = np.random.exponential(0, size=N * (N - 1) // 2,)
Z = linkage(mat)
tree = hier.tree_from_linkage(Z)


def setup():
    dw.set_root(tree)


def run():
    dw.clear()


res = timeit.timeit(
    "run()", setup="setup()", globals=globals(), number=100
)
print(res)

Before: 17.68
After: 0.71

@codecov
Copy link

codecov bot commented Jan 23, 2020

Codecov Report

Merging #4361 into master will decrease coverage by <.01%.
The diff coverage is 90.9%.

@@            Coverage Diff             @@
##           master    #4361      +/-   ##
==========================================
- Coverage   86.91%   86.91%   -0.01%     
==========================================
  Files         396      396              
  Lines       71991    71996       +5     
==========================================
+ Hits        62571    62575       +4     
- Misses       9420     9421       +1

@janezd janezd self-assigned this Jan 24, 2020
@janezd janezd merged commit 148959e into biolab:master Jan 24, 2020
@ales-erjavec ales-erjavec deleted the owhierclust-clear-opt branch January 24, 2020 15:17
ales-erjavec added a commit to ales-erjavec/orange3 that referenced this pull request Jan 31, 2020
ales-erjavec added a commit to ales-erjavec/orange3 that referenced this pull request Jan 31, 2020
... instead of installing individual filters on every item. This is
a much better fix then biolabgh-4361
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.

2 participants