[FIX] OWMosaic: Fix crash for empty column #2006
Conversation
Orange/widgets/visualize/owmosaic.py
Outdated
| @@ -400,7 +400,7 @@ def set_data(self, data): | |||
| self.closeContext() | |||
| self.data = data | |||
| self.init_combos(self.data) | |||
| if self.data is None or not len(self.data): | |||
| if self.data is None or not len(self.data) or not len(self.data[0]): | |||
There was a problem hiding this comment.
I think it would be better to test this on domain, not data.
Besides, meta attributes are useful only if they are discrete or continuous, not strings. I think you should check this, too.
| def test_empty_column(self): | ||
| """Check that the widget doesn't crash if the columns are empty""" | ||
| self.send_signal("Data", self.data[:,:0]) | ||
|
|
There was a problem hiding this comment.
Add a test with a single string meta attribute.
rokgomiscek
force-pushed
the
mosaic_no_features
branch
from
8103912
to
21d3da1
Compare
Codecov Report
@@ Coverage Diff @@
## master #2006 +/- ##
=========================================
+ Coverage 70.65% 70.7% +0.05%
=========================================
Files 315 343 +28
Lines 53828 54484 +656
=========================================
+ Hits 38034 38525 +491
- Misses 15794 15959 +165Continue to review full report at Codecov.
|
| def test_string_meta(self): | ||
| """Check widget for dataset with only one string meta""" | ||
| domain = Domain([], metas=[StringVariable("m")]) | ||
| data = Table(domain, np.arange(6).reshape(6, 1)[:, :0], |
There was a problem hiding this comment.
Is np.arange(6).reshape(6, 1)[:, :0] supposed to produce np.empty((6, 0)) ?
There was a problem hiding this comment.
Yes.... changed :)
Orange/widgets/visualize/owmosaic.py
Outdated
| @@ -400,7 +400,8 @@ def set_data(self, data): | |||
| self.closeContext() | |||
| self.data = data | |||
| self.init_combos(self.data) | |||
| if self.data is None or not len(self.data): | |||
| if self.data is None or not len(self.data) or not \ | |||
| any(attr.is_discrete or attr.is_continuous for attr in chain(data.domain, data.domain.metas)): | |||
There was a problem hiding this comment.
Broke the line.
| @@ -26,6 +26,10 @@ def test_no_data(self): | |||
| """Check that the widget doesn't crash on empty data""" | |||
| self.send_signal("Data", self.data[:0]) | |||
|
|
|||
| def test_empty_column(self): | |||
| """Check that the widget doesn't crash if the columns are empty""" | |||
| self.send_signal("Data", self.data[:,:0]) | |||
There was a problem hiding this comment.
Lint would like one space after commas.
rokgomiscek
force-pushed
the
mosaic_no_features
branch
from
7df0149
to
ce531e3
Compare
|
Lint is still not happy... |
rokgomiscek
force-pushed
the
mosaic_no_features
branch
from
ce531e3
to
4cdd105
Compare
|
Better? :) |
Orange/widgets/visualize/owmosaic.py
Outdated
| @@ -400,7 +400,10 @@ def set_data(self, data): | |||
| self.closeContext() | |||
| self.data = data | |||
| self.init_combos(self.data) | |||
| if self.data is None or not len(self.data): | |||
| if (self.data is None or | |||
There was a problem hiding this comment.
Almost there :) Now radon is complaining that this function (which was not exactly a diamond before) looks worse.
I tend to agree, as the data checking part of code is growing larger and larger. I would recommend extraction the whole if-elif-else block into a method called something like "_get_discrete_data" that gets the data and returns data that consists only of discrete variables. The whole block would then turn into self.discrete_data = self._get_discrete_data(data)
+points if the new method has a docstring :)
There was a problem hiding this comment.
Moved the block to a new method (with a docstring).
rokgomiscek
force-pushed
the
mosaic_no_features
branch
from
4cdd105
to
9ff33e0
Compare
Issue
Widget crashes if the columns are empty.
To reproduce: Connect data to Select Columns. Remove features, target variable and meta attributes. Connect to Mosaic Display.
Description of changes
Added an extra check in in set_data().
Includes