[ENH] Venn Diagram: Add relations over columns, simplify over rows #4006
Conversation
AndrejaKovacic
force-pushed
the
venn
branch
2 times, most recently
from
c9f1236 to
33c82c4
Compare
Codecov Report
@@ Coverage Diff @@
## master #4006 +/- ##
==========================================
+ Coverage 85.95% 86.17% +0.22%
==========================================
Files 393 394 +1
Lines 70128 70305 +177
==========================================
+ Hits 60277 60584 +307
+ Misses 9851 9721 -130 |
AndrejaKovacic
force-pushed
the
venn
branch
3 times, most recently
from
bba054d to
9a15d0d
Compare
AndrejaKovacic
force-pushed
the
venn
branch
2 times, most recently
from
2907599 to
314f5e7
Compare
|
TL;DR: Checkout my commit and see what I've done. :| I'm sorry... There was a huge gap between the last radio button and the combo. I tried removing it and noticed there's an unnecessary box. And that the widget uses attribute indices althought the combo box uses a model. After fixing this, I realized that radio buttons are not necessary if we just use "Instance identity" as a placeholder for None. So, changing this, I noticed that the combo is disabled unless the diagram shows rows, hence, the combo actually belongs to one of the options in the other radio button group. This made the control area almost empty, in particular when I removed the empty and useless info box. So we arrived to what I've just committed. |
|
|
janezd
force-pushed
the
venn
branch
2 times, most recently
from
9b4b324 to
6ff92d0
Compare
|
@AndrejaKovacic, in your last commit you removed context settings. You decided you didn't like them? :) (I've modified the commit to put them back.) |
janezd
changed the title
Co-authored-by: Andreja Kovacic <[email protected]>
| self._invalidate() | ||
| self._updateItemsets() | ||
| def _on_matching_changed(self): | ||
| self.output_duplicates_box.setEnabled(bool(self.rowwise)) |
There was a problem hiding this comment.
Errrr, I don't get why the whole Output box would be disabled for Columns - including autocommit!
Probably only the Output duplicates option should be?
Edit: maybe this bug also hints at non-optimal naming - output_duplicates_box could be just output_box (duplicates setting is only one part of it).
| domains = [input.table.domain for input in self.data.values()] | ||
| self.samedomain = all((d1 == d2) for d1, d2 in pairwise(domains)) | ||
| if not self.samedomain: | ||
| return self.settings_incompatible() |
There was a problem hiding this comment.
The domains do not need to match. That is why there is an option to change Rows, matched by: from Instance identity (where it makes sense to match domains first) to a single variable with unique identifiers (matching is done only on that attribute, the rest of the domains can differ).
To test use the zoo dataset, connect to Venn. Also connect to PCA->Scatter plot, select some points and connect to Venn. Venn should be able to match on attribute Name.
| @@ -687,7 +596,7 @@ def __hash__(self): | |||
| def __eq__(self, other): | |||
| # XXX: comparing NaN with different payload | |||
| return (isinstance(other, ComparableInstance) | |||
| and domain_eq(self.domain, other.domain) | |||
| and (self.domain == other.domain) | |||
There was a problem hiding this comment.
This now compares metas as well (before we didn't). Intentional?
There was a problem hiding this comment.
It was requested in the original issue, that domains match completely, when doing comparisons over rows. This is a remnant of that, but as we later decided against the idea, i should have reverted it. Good catch.
|
The mentioned problems seem to be fixed. I found a new one, and the documentation update is still needed (can be done last).
|
lanzagar
changed the title
Issue
Implements #3991.
Description of changes
Venn diagram over rows is now simpler. Venn diagram over columns is now possible. I would appreciate comments about output, especially when using columns.
Includes