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] owlouvainclustering: Fix race conditions #3322

Merged
merged 5 commits into from
Oct 26, 2018

Conversation

ales-erjavec
Copy link
Contributor

Issue

There are race conditions in the Louvain clustering widget.

The pca_projection, graph and partitions attributes are accessed and set directly from threads. In particular from the already 'cancelled' jobs. The code does not wait for cancelled jobs to finish, but they do modify the computed partial results. This can lead to undefined final results
on the output.

I can trigger this reliably by:
screen shot 2018-10-19 at 11 24 41

File: wdbc.tab
Louvain Clustering (both):

  • Apply PCA is checked: 20 components
  • Euclidean distance
  • k neighbors: 10
  • Resolution 2.5
  • Apply automatically is checked

Python script contents:

import numpy as np
d1, d2 = in_datas

c1, _ = d1.get_column_view("Cluster")
c2, _ = d2.get_column_view("Cluster")
assert np.array_equal(c1, c2)

In one of the Louvain clustering widgets tab to k neighbors spin box and enter 50, then quickly press down key on the keyboard a few times then even quicker enter 10 (you might copy 10 to clipboard and just paste it).

Description of changes
  • Refactor the code to eliminate race conditions in the access/setting of pca_projection, graph and partitions.

  • Limit the number of executing tasks to one.

Includes
  • Code changes
  • Tests
  • Documentation

@ales-erjavec ales-erjavec force-pushed the fixes/louvain-threading branch from eddc60c to 5a8ed9b Compare October 19, 2018 11:31
@codecov
Copy link

codecov bot commented Oct 19, 2018

Codecov Report

Merging #3322 into master will increase coverage by 0.03%.
The diff coverage is 92.8%.

@@            Coverage Diff            @@
##           master   #3322      +/-   ##
=========================================
+ Coverage   82.06%   82.1%   +0.03%     
=========================================
  Files         351     351              
  Lines       61898   62076     +178     
=========================================
+ Hits        50799   50965     +166     
- Misses      11099   11111      +12

@ales-erjavec ales-erjavec force-pushed the fixes/louvain-threading branch 4 times, most recently from ac27b68 to e533a75 Compare October 22, 2018 10:36
@ales-erjavec ales-erjavec force-pushed the fixes/louvain-threading branch from e533a75 to 95069b2 Compare October 25, 2018 09:51
@lanzagar lanzagar added this to the 3.17 milestone Oct 26, 2018
@lanzagar lanzagar closed this Oct 26, 2018
@lanzagar lanzagar reopened this Oct 26, 2018
@lanzagar lanzagar merged commit 0c3958e into biolab:master Oct 26, 2018
@ales-erjavec ales-erjavec deleted the fixes/louvain-threading branch May 20, 2019 11:31
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