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] Silhouette Plot: now setting axis range properly #2377

Merged
merged 2 commits into from
Jun 26, 2017
Merged

[FIX] Silhouette Plot: now setting axis range properly #2377

merged 2 commits into from
Jun 26, 2017

Conversation

jerneju
Copy link
Contributor

@jerneju jerneju commented Jun 5, 2017

Issue

Handling nan values.
https://sentry.io/biolab/orange3/issues/220334718/
screenshot_20170605_161318

Description of changes
Includes
  • Code changes
  • Tests
  • Documentation

@codecov-io
Copy link

codecov-io commented Jun 5, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@942877a). Click here to learn what that means.
The diff coverage is 73.68%.

@@            Coverage Diff            @@
##             master    #2377   +/-   ##
=========================================
  Coverage          ?   74.33%           
=========================================
  Files             ?      320           
  Lines             ?    55828           
  Branches          ?        0           
=========================================
  Hits              ?    41497           
  Misses            ?    14331           
  Partials          ?        0

@@ -574,11 +574,11 @@ def clear(self):

def __setup(self):
# Setup the subwidgets/groups/layout
smax = max((numpy.max(g.scores) for g in self.__groups
smax = max((numpy.max(numpy.nan_to_num(g.scores)) for g in self.__groups
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not numpy.nanmax (here) and numpy.nanmin (below)? Plus, set it to 1 or -1 if the length is zero or all values are NaN.

@jerneju jerneju changed the title [FIX] Silhouette Plot: now setting axis range properly [WIP][FIX] Silhouette Plot: now setting axis range properly Jun 13, 2017
@janezd
Copy link
Contributor

janezd commented Jun 23, 2017

Is this still WIP, as the title suggests, or can we merge it?

As a sidenote, the snapshot again provides no information (except that the bug can be reproduced by connecting Silhouette to File), so one has to check the code to see what the PR is about. No need to fix this here, but please provide meaningful descriptions in the future.

@janezd
Copy link
Contributor

janezd commented Jun 23, 2017

You can add # pylint disable=protected-access at the top of the tests. Tests are allowed to access anything. This will decrease the lint error count.

@jerneju jerneju changed the title [WIP][FIX] Silhouette Plot: now setting axis range properly [FIX] Silhouette Plot: now setting axis range properly Jun 23, 2017
@jerneju
Copy link
Contributor Author

jerneju commented Jun 23, 2017

Ok, fixed.

@@ -1,3 +1,4 @@
# pylint disable=protected-access
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for my typeo: this has no effect due to a missing a colon after pylint (see below). You can also add a comment # Test methods are allowed to access private members above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, my mistake, I should have known by now that there is a colon after # pylint.

@janezd janezd merged commit b513d40 into biolab:master Jun 26, 2017
@jerneju jerneju deleted the exception-silhouette branch June 26, 2017 12:33
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