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] Feature statistics fixes #3480

Merged
merged 7 commits into from
Jan 18, 2019

Conversation

pavlin-policar
Copy link
Collaborator

Issue
  1. Feature Statistics would crash when it got a discrete variable with no values
  2. Feature Statistics would crash when switching from a data set with a target variable to a data set with no target variable
Description of changes
  1. Fix. This crashed because DiscreteVariable.str_val crashes if it has no values. This means that the problem may exist in other widgets as well. I thought about fixing the str_val implementation, but I don't really know what it should do in this case (implementation here). In both cases a string is returned, so it's not clear what to do here. Perhaps this was even left out by design, so I fixed the issue on the widget.
  2. Fix.
  3. Update tests to use new signal style i.e. self.send_data("Data", data) to self.send_data(self.widget.Inputs.data, data).
Includes
  • Code changes
  • Tests
  • Documentation

@codecov
Copy link

codecov bot commented Dec 15, 2018

Codecov Report

Merging #3480 into master will increase coverage by 0.15%.
The diff coverage is 95.34%.

@@            Coverage Diff             @@
##           master    #3480      +/-   ##
==========================================
+ Coverage   83.55%   83.71%   +0.15%     
==========================================
  Files         367      370       +3     
  Lines       65454    66299     +845     
==========================================
+ Hits        54690    55499     +809     
- Misses      10764    10800      +36

@@ -500,7 +500,7 @@ def data(self, index, role):
if role == Qt.DisplayRole:
if isinstance(attribute, DiscreteVariable):
output = self._center[row]
if not np.isnan(output):
Copy link
Contributor

Choose a reason for hiding this comment

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

What about else? Should output be set to "" in this case? np.nan is properly handled below with if output==output: output = "", but this won't work if an attribute has no values and self._center[row] is ... whatever it is in this case (probably 0).

@pavlin-policar
Copy link
Collaborator Author

I've removed the commit that handled discrete variables with no values since this should never happen. In the event that it does happen, the underlying issue should be fixed and patched up in this widget.

@lanzagar
Copy link
Contributor

lanzagar commented Jan 4, 2019

I am not sure why this passes on travis, but fails on appveyor (maybe different versions of libraries?).
But the error there looks relevant. Can you take a look and try to fix whatever is causing the problem?

@janezd janezd assigned thocevar and unassigned lanzagar Jan 11, 2019
@thocevar
Copy link
Contributor

I can reproduce appveyor's error on windows. test_owfeaturestatistics -> force_render_table -> FeatureStatisticsTableModel.data tries to get a string value of a DiscreteVariable that has an empty list of values (owfeaturestatistics.py, line 515).

@pavlin-policar
Copy link
Collaborator Author

Yes, that's actually the problem. Building a DiscreteVariable with no values shouldn't be possible, and apparently, this does not happen on non-Windows systems (for some reason). It's fairly easy to fix the problem here and check for such an event, but that would mask the underlying issue i.e. a discrete variable with no possible values!

@thocevar
Copy link
Contributor

We traced the problem to scipy's mode function. For an array of nans it should return nan. It does so in the latest version 1.2, but not in version 1.0 that's used by appveyor.

@janezd janezd merged commit e5a5329 into biolab:master Jan 18, 2019
@pavlin-policar pavlin-policar deleted the feature-statistics-fixes branch January 19, 2019 19:47
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