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] KMeans: should not crash when there is less data rows than k #2172

Merged
merged 1 commit into from
Apr 13, 2017
Merged

[FIX] KMeans: should not crash when there is less data rows than k #2172

merged 1 commit into from
Apr 13, 2017

Conversation

jerneju
Copy link
Contributor

@jerneju jerneju commented Mar 31, 2017

Issue

When there is less rows in a data than selected (from - to, second option) the widget crashes.
https://sentry.io/biolab/orange3/issues/235248479/

Description of changes
Includes
  • Code changes
  • Tests
  • Documentation

@jerneju
Copy link
Contributor Author

jerneju commented Mar 31, 2017

@codecov-io
Copy link

codecov-io commented Mar 31, 2017

Codecov Report

Merging #2172 into master will increase coverage by 0.16%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #2172      +/-   ##
=========================================
+ Coverage   71.44%   71.6%   +0.16%     
=========================================
  Files         268     318      +50     
  Lines       53093   54527    +1434     
=========================================
+ Hits        37932   39045    +1113     
- Misses      15161   15482     +321

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 54d8498...ed43b68. Read the comment docs.

Copy link
Contributor

@janezd janezd left a comment

Choose a reason for hiding this comment

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

This PR breaks the widget. Just open it, enable optimization, and decrease To. The number of rows in the table doesn't change.

The problem is that return in line 217 should also be changed to return True.

As a punishment :), add a test that check this (e.g. a tests that currently fails if the user does what I described above) before fixing this problem. :)

self.run_optimization()
self.mainArea.show()
self.update_results()
if self.run_optimization():
Copy link
Contributor

Choose a reason for hiding this comment

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

Say that mainArea is hidden. If optimization fails, main area won't be shown and updated. This is OK.

What if mainArea is currently shown and contains some data? If optimization fails, it won't be hidden, but it won't update either...

@astaric astaric added this to the 3.4.2 milestone Apr 7, 2017
@astaric astaric self-assigned this Apr 7, 2017
@janezd janezd merged commit 1794e3e into biolab:master Apr 13, 2017
@jerneju jerneju deleted the key-kmeans branch April 21, 2017 14:21
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.

4 participants