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] OWLouvain: Ensure deterministic clustering #3492

Merged
merged 1 commit into from
Dec 26, 2018

Conversation

pavlin-policar
Copy link
Collaborator

Issue

Fixes #3278.

Description of changes

Version 0.12 of python-louvain was just released and with it the random_state parameter, which was missing before. I've added the parameter to our wrapper and set the random_state to a fixed value in the widget, so the results are deterministic in the widget.

Includes
  • Code changes
  • Tests
  • Documentation

@pavlin-policar
Copy link
Collaborator Author

I've also fixed the version to 0.12 because the API might change in the next version taynaud/python-louvain#23.

@janezd
Copy link
Contributor

janezd commented Dec 20, 2018

Fixing a version looks like a good idea (as anybody who just spent a day trying to find why tests suddenly fail would confirm), yet it may be better to fix problems as they come, instead of jumping a few versions at some later time and having to fix the whole heap then.

I would let it fail when the new version comes and just change the import name when it happens. But it would be a good idea to add a comment at the point where we'll have to rename the import.

@janezd janezd self-assigned this Dec 20, 2018
@pavlin-policar pavlin-policar force-pushed the louvain-random-state branch 2 times, most recently from 0d2f592 to a27031b Compare December 21, 2018 08:35
@pavlin-policar
Copy link
Collaborator Author

I've removed the version from the requirements and added a note to the import.

@codecov
Copy link

codecov bot commented Dec 22, 2018

Codecov Report

Merging #3492 into master will decrease coverage by 0.03%.
The diff coverage is 90%.

@@            Coverage Diff             @@
##           master    #3492      +/-   ##
==========================================
- Coverage   83.59%   83.55%   -0.04%     
==========================================
  Files         367      367              
  Lines       65570    65464     -106     
==========================================
- Hits        54814    54700     -114     
- Misses      10756    10764       +8

@codecov
Copy link

codecov bot commented Dec 22, 2018

Codecov Report

Merging #3492 into master will increase coverage by <.01%.
The diff coverage is 90%.

@@            Coverage Diff            @@
##           master   #3492      +/-   ##
=========================================
+ Coverage   83.59%   83.6%   +<.01%     
=========================================
  Files         367     367              
  Lines       65570   65580      +10     
=========================================
+ Hits        54814   54827      +13     
+ Misses      10756   10753       -3

@janezd janezd merged commit 3541b61 into biolab:master Dec 26, 2018
@pavlin-policar pavlin-policar deleted the louvain-random-state branch December 26, 2018 15:56
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.

Set a fixed random seed in Louvain Clustering
2 participants