[ENH] Visualize widgets: Output Annotated data and Fixups#1677
[ENH] Visualize widgets: Output Annotated data and Fixups#1677janezd merged 11 commits intobiolab:masterfrom
Conversation
| if self.append_clusters: | ||
| def remove_other_value(vars_): | ||
| vars_ = [var for var in vars_] | ||
| vars_ = list(vars_).copy() |
There was a problem hiding this comment.
Ooops, in my earlier comment I thought that vars_ was a list. Sorry. Now that you have to convert it to a list anyway, list(vars_) will suffice; you can skip copy().
VesnaT
force-pushed
the
annotated_data
branch
from
d9eec0f to
987e0ca
Compare
Current coverage is 89.39% (diff: 100%)@@ master #1677 diff @@
==========================================
Files 79 79
Lines 8603 8603
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
Hits 7691 7691
Misses 912 912
Partials 0 0
|
janezd
left a comment
janezd
left a comment
There was a problem hiding this comment.
Nice work. Adding the signal to the widget is really smooth (and easy to check :), and you had the patience to write some reasonable tests, too.
| all((var in [var.name for var in annotated.domain.metas] | ||
| for var in [var.name for var in selected.domain.metas]))) | ||
| self.assertLess(set(var.name for var in selected.domain.metas), | ||
| set(var.name for var in annotated.domain.metas)) |
There was a problem hiding this comment.
No need to change anything here, but: you can also use set comprehensions in such cases. (I keep forgetting this, too.)
| # if hasattr(model, 'meta_depth_limit'): | ||
| # self.depth_limit = model.meta_depth_limit | ||
| # self.update_depth() | ||
| self.send(ANNOTATED_DATA_SIGNAL_NAME, |
There was a problem hiding this comment.
Is this needed? The "Selected data" signal is not sent here. I commented it out and it seems it still works.
| data = self.tree_adapter.get_instances_in_nodes( | ||
| self.clf_dataset, [item.tree_node.label for item in items]) | ||
| self.send('Selected Data', data) | ||
| nodes = [i.tree_node.label for i in self.scene.selectedItems() |
There was a problem hiding this comment.
Isn't this already computed above, as the second argument in the call of get_instances_in_nodes?
|
|
||
| data_output_identifier = "Filtered data" | ||
| outputs = [(data_output_identifier, Table)] | ||
| data_output_identifier = "Selected Data" |
There was a problem hiding this comment.
I hate this.
First, it should be upper case -- if for nothing else, to match the case of ANNOTATED_DATA_SIGNAL_NAME.
Second, it's probably the only widget which defines a constant for the signal name. This could be a good idea, but only if we do it elsewhere too.
It's not your fault (it was here before), and removing it is not a part of this PR. But since you have to rename it anyway, you could perhaps simply get rid of it. Just a suggestion, you don't have to.
VesnaT
force-pushed
the
annotated_data
branch
from
987e0ca to
2f7246a
Compare
|
@VesnaT, a small rebase, please. |
VesnaT
force-pushed
the
annotated_data
branch
2 times, most recently
from
cc350f6 to
86e2ebd
Compare
VesnaT
force-pushed
the
annotated_data
branch
from
86e2ebd to
635ba39
Compare
4 participants
No description provided.