[ENH] Mosaic: Selectable color variable by jerneju · Pull Request #2133 · biolab/orange3

jerneju · 2017-03-23T10:05:24Z

Issue

Mosaic plot uses only class variable to color the visualization.
#2036

Description of changes

A new option added to the control area that allows the user to change the variable used for coloring. It defaults to class_var when it is available.

Includes

Code changes
Tests
Documentation

codecov-io · 2017-03-23T10:41:51Z

Codecov Report

Merging #2133 into master will increase coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2133      +/-   ##
==========================================
+ Coverage   72.49%   72.55%   +0.05%     
==========================================
  Files         319      319              
  Lines       55241    55314      +73     
==========================================
+ Hits        40049    40133      +84     
+ Misses      15192    15181      -11

janezd · 2017-03-23T21:58:08Z

Orange/widgets/visualize/owmosaic.py

+            elif data.domain.attributes[0]:
+                color_var = data.domain.attributes[0]
+            else:
+                color_var = data.domian.metas[0]


domian? Add tests for this.

What if the first meta attribute is a string?

If the first meta attribute is a string, nothing happens. But I am still testing.

janezd · 2017-03-23T22:02:45Z

Orange/widgets/visualize/owmosaic.py

+    variable_color = ContextSetting("", exclude_metas=False)
    selection = ContextSetting(set())

+    color_var = None


This does work, but we define instance attributes in __init__. Settings are an exception.

... if this line is needed at all. I may be stupid, by I don't see what color_var is doing and whether it's a string or a list of strings.

It is not needed, I thought that I needed it, but then I have forgotten to erase it. Any yes, Lint reports that this variable is unused.

jerneju · 2017-03-24T09:20:24Z

Vizrank has to be changed. At the moment it still uses class variable and not color.

janezd · 2017-03-31T19:57:29Z

Orange/widgets/visualize/owmosaic.py

+            box2, self, "variable_color",
+            callback=self.update_colors,
+            model=self.color_model, **common_options)
+        """


Why don't you actually use a DomainModel here? Use "Pearson residuals" as placeholder, perhaps followed by a class and then other attributes. If Pearson is a placeholder, self.variable_color will be None when the user selects it.

If you for any reason can't use the domain model, remove this comment.

janezd · 2017-03-31T20:06:51Z

Orange/widgets/visualize/owmosaic.py

+        metas = [v for v in self.data.domain.metas if v != color_var]
+        domain = Domain(attributes, class_vars, metas)
+        data = self.data.from_table(domain, self.data)
+        self.set_data(data)


Do I see correctly that the combo calls update_colors, which calls change_domain, which in turn calls set_data? And set_data is a callback for signal Data, which ... basically resets the widget, including closing the context and so forth?

I think widgets should never call signal handlers like set_data. If set_data does some things that need to be done when changing the combo (like stopping the vizrank, updating the graph), extract those into a separate function and call it from set_data and from update_colors (or change_domain).

Besides, remove change_domain altogether. The widget should output the same domain. Changing the coloring must not replace the class attribute, it should only affect the visual appearance.

I think that changing the color should retain the selection.

janezd · 2017-03-31T20:15:22Z

Orange/widgets/visualize/owmosaic.py

+            orientation=Qt.Horizontal, contentsLength=12,
+            callback=self.update_colors,
+            sendSelectedValue=True, valueType=str)
+


The changed GUI is confusing.

If the combo is set to None, the two radio buttons below have no effect.

If the radio button is set to "Pearson", then the combo has no effect.

Replace the None option in the combo with Pearson residuals

You can then eliminate the radio buttons.

The checkbox should then be active (only if) combo is not Pearson residuals. If you use DomainModel, you will check for this with, simply, self.variable_color is not None.

janezd · 2017-03-31T20:17:21Z

Orange/widgets/visualize/owmosaic.py

    variable2 = ContextSetting("", exclude_metas=False)
    variable3 = ContextSetting("", exclude_metas=False)
    variable4 = ContextSetting("", exclude_metas=False)
+    variable_color = ContextSetting("(No class)", exclude_metas=False)


If you use a model (see below), change the default to None.

janezd · 2017-03-31T20:20:10Z

Also, please rebase.

lanzagar · 2017-04-25T10:34:21Z

Orange/widgets/visualize/owmosaic.py

                        self.variable3, self.variable4]
-            if a and a != "(None)"]
+            if a and a != "(None)"], \
+               self.variable_color if self.variable_color != "(Pearson residuals)" else None


It looks to me like this method is now only called in one additional place (L484), where you discard the first list of attributes and only use the color variable.
Why don't you just use self.variable_color there, and you even have an if already (just change "is None" to "== Pearson")

I would, however, prefer to put the string "(Pearson residuals)" in a constant at the top and use that. You currently already use the hard-coded string 7 times in this PR.

lanzagar · 2017-04-25T10:47:05Z

Orange/widgets/visualize/owmosaic.py

                for combo in self.attr_combos:
                    combo.addItem(icons[attr], attr.name)
+        self.cb_attr_color.addItem("(Pearson residuals)")
+        for attr in chain(data.domain.class_vars, data.domain.attributes, data.domain.metas):


IIRC we wanted to transition to a new order in dropdowns of: class_vars, metas, attributes?
Since some data can have thousands of attributes and scrolling to the end can be a pain.
To be double-checked.

Even better - use DomainModel. I think it should be possible to add a custom string at the beginning.

Yes, DomainModel enables us to add a custom string at the beginning. Now class variables, attributes and metas are added into a combo in an alphabetical order.

lanzagar · 2017-04-25T10:47:33Z

Orange/widgets/visualize/owmosaic.py

                    combo.addItem(icons[attr], attr.name)
+        self.cb_attr_color.addItem("(Pearson residuals)")
+        for attr in chain(data.domain.class_vars, data.domain.attributes, data.domain.metas):
+            if attr.is_discrete or attr.is_continuous:


attr.is_primitive

lanzagar · 2017-04-25T10:49:38Z

Orange/widgets/visualize/owmosaic.py

+            color_var = self.data.domain[variable_color]
+            self.interior_coloring = self.CLASS_DISTRIBUTION
+            self.bar_button.setEnabled(True)
+        class_vars = color_var


just use color_var?

GH-2133 GH-2036

kernc · 2017-05-05T17:30:05Z

Orange/widgets/visualize/owmosaic.py

-            self.interior_coloring_opts, box="Interior Coloring",
-            callback=self.coloring_changed)
+        box2 = gui.vBox(self.controlArea, box="Interior Coloring")
+        dmod = DomainModel


Please tell more.

biolabGH-2133 biolabGH-2036

janezd reviewed Mar 23, 2017

View reviewed changes

janezd requested changes Mar 31, 2017

View reviewed changes

jerneju changed the title ~~Mosaic: change the variable used for coloring~~ [WIP] Mosaic: change the variable used for coloring Apr 14, 2017

lanzagar requested changes Apr 25, 2017

View reviewed changes

lanzagar self-assigned this Apr 25, 2017

Mosaic - change the variable used for coloring

534ecb6

GH-2133 GH-2036

lanzagar approved these changes May 5, 2017

View reviewed changes

lanzagar changed the title ~~[WIP] Mosaic: change the variable used for coloring~~ [FIX] Mosaic: Selectable color variable May 5, 2017

lanzagar changed the title ~~[FIX] Mosaic: Selectable color variable~~ [ENH] Mosaic: Selectable color variable May 5, 2017

lanzagar merged commit 1690956 into biolab:master May 5, 2017

jerneju mentioned this pull request May 5, 2017

Color attribute selection in Mosaic #2036

Closed

jerneju deleted the gh-2036-mosaic branch May 5, 2017 14:58

kernc reviewed May 5, 2017

View reviewed changes

mstrazar pushed a commit to mstrazar/orange3 that referenced this pull request May 6, 2017

Mosaic - change the variable used for coloring

2130ec3

biolabGH-2133 biolabGH-2036

mstrazar pushed a commit to mstrazar/orange3 that referenced this pull request May 6, 2017

Mosaic - change the variable used for coloring

f137056

biolabGH-2133 biolabGH-2036

astaric mentioned this pull request May 8, 2017

Sparse concatenate #2286

Merged

3 tasks

Uh oh!

Conversation

jerneju commented Mar 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Description of changes

Includes

Uh oh!

codecov-io commented Mar 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerneju Mar 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerneju commented Mar 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janezd Mar 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janezd commented Mar 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jerneju commented Mar 23, 2017 •

edited

Loading

codecov-io commented Mar 23, 2017 •

edited

Loading

jerneju Mar 24, 2017 •

edited

Loading

janezd Mar 31, 2017 •

edited

Loading