[FIX] Mosaic Display: subset data #2528
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2528 +/- ##
==========================================
- Coverage 75.15% 75.14% -0.02%
==========================================
Files 324 324
Lines 56963 56973 +10
==========================================
+ Hits 42812 42813 +1
- Misses 14151 14160 +9 |
|
Feed a dataset and a subset from File to Mosaic. Then change the data set in File. The widget crashes. The problem is that |
Orange/widgets/visualize/owmosaic.py
Outdated
| @@ -837,7 +841,7 @@ def line(x1, y1, x2, y2): | |||
| if sum(counts) == 1: | |||
There was a problem hiding this comment.
Marking the whole rectangle is ugly, unlikely and non-obvious (despite being here for at least three years). I suggest removing this if and its contents altogether.
Orange/widgets/visualize/owmosaic.py
Outdated
| @@ -461,6 +462,9 @@ def set_subset_data(self, data): | |||
| except: | |||
| self.subset_data = None | |||
| self.Warning.incompatible_subset(shown=data is not None) | |||
| finally: | |||
| indices = list(set(e.id for e in self.subset_data)) | |||
There was a problem hiding this comment.
indices = {e.id for e in self.subset_data}
That is, use set comprehension and, more importantly, indices should be a set.
There was a problem hiding this comment.
I would set self.subset_indices to None if self.data is None. Alternatively, this twisted code (not by your fault!) could be improved. I think that set_subset_data should be only:
@Inputs.data_subset
def set_subset_data(self, data):
self.subset_data = data
The rest of this function belongs to handleNewSignals (or a separate method called by handleNewSignals). This is precisely what handleNewSignals is meant for. (It's possible that this widget was originally written before we had handleNewSignals.)
Orange/widgets/visualize/owmosaic.py
Outdated
| try: | ||
| indices = {e.id for e in self.subset_data.transform(self.data.domain)} | ||
| except: | ||
| self.Warning.incompatible_subset(shown=self.subset_indices is not None) |
There was a problem hiding this comment.
Have I overlooked something, or is self.subset_indices always None here? Originally, this was data is not None, but here even this is not needed since data is checked to be non-None.
Second, where can this try clause fail at all?
Perhaps add a test that feeds iris as data and titanic as subset, and check that the warning is displayed. (Currently it's not - I tried.)
A general problem: how do we defined an "incompatible subset"? I guess a subset is incompatible if none of the attributes can be transformed, that is, if transformed = self.subset_data.transform(self.data.domain) and np.all(np.isnan(transformed.X)) and np.all(np.isnan(transformed.Y)).
There was a problem hiding this comment.
Maybe Table.transform should have an additional argument, a flag that would tell it to raise an exception if no attribute can be transformed. Table.transform can probably easily detect this (I haven't checked), it's faster than np.all(np.isnan(..)) and, most importantly, does not report an incompatible domain when the domain is compatible (maybe even the same), but all data is missing.
Orange/widgets/visualize/owmosaic.py
Outdated
| indices = {e.id for e in transformed} | ||
| self.subset_indices = [ex.id in indices for ex in self.data] | ||
|
|
||
| self.clear_selection() |
There was a problem hiding this comment.
This is new. Is it necessary -- and desired?
Selection is already cleared upon receiving new data. This would also clear it upon receiving a subset, which is unnecessary and undesired, I guess. Have I overlooked something?
Orange/widgets/visualize/owmosaic.py
Outdated
| @@ -482,6 +479,7 @@ def reset_graph(self): | |||
|
|
|||
| def set_color_data(self): | |||
| if self.data is None or len(self.data) < 2 or len(self.data.domain.attributes) < 1: | |||
| self.coloring_changed() | |||
There was a problem hiding this comment.
Is this necessary? It would stop vizrank, which can't be running in this case anyway, and it updates graph, which is also unnecessary.
|
Everything looks ok, I'd merge ... except that appveyor stopped. @astaric? |
|
@janezd : Appveyor succeeded. |
Issue
Fixes #2515 .
Description of changes
Includes