[ENH] OWTranspose: Add a new widget by VesnaT · Pull Request #1738 · biolab/orange3

VesnaT · 2016-11-10T11:12:35Z

Issue

Transpose widget

Description of changes

Includes

Code changes
Tests
Documentation

codecov-io · 2016-11-10T11:25:25Z

Current coverage is 88.92% (diff: 100%)

Merging #1738 into master will increase coverage by 0.06%

@@             master      #1738   diff @@
==========================================
  Files            82         82          
  Lines          8896       8952    +56   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           7905       7961    +56   
  Misses          991        991          
  Partials          0          0

Powered by Codecov. Last update 1cf3e90...bad6ed1

janezd

I have a few minor comments and suggestion (DomainModel would really simplify the widget, I guess), and one related to GUI itself.

As always, I appreciate the effort you put in tests - especially since I know how annoying it is to write them.

janezd · 2016-11-11T16:25:02Z

Orange/data/table.py

+                        else variable.repr_val(row[i])
+
+                    if value not in MISSING_VALUES:
+                        attributes[j].attributes.update({variable.name: value})


Isn't this the same as attributes[j].attributes[variable.name] = value?

janezd · 2016-11-11T16:29:34Z

Orange/data/table.py

+        self.X = table.X.T
+        attributes = [ContinuousVariable(str(row[feature_names_column]))
+                      for row in table] if feature_names_column else \
+            [ContinuousVariable("Feature" + str(i + 1)) for i in range(n_cols)]


Add a space between Feature and number. Consider also putting the necessary number of leading zeros before the number (format to 1 + int(ceil(log(n_cols, 10))) places).

janezd · 2016-11-11T16:31:54Z

Orange/data/table.py

        return contingencies, unknown_rows

+    @classmethod
+    def transpose(cls, table, feature_names_column="", meta_attr_name="Meta1",


"Meta1" is rather uninformative name for a meta attribute. :) Should we call it "Feature name" instead?

janezd · 2016-11-11T16:39:22Z

Orange/data/table.py

+                                  dtype=object)
+            metas.append(StringVariable(meta_attr_name))
+
+        names = chain.from_iterable(list(attr.attributes.keys())


Isn't list(attr.attributes.keys()) the same as list(attr.attributes)? (I don't like calling keys(), but it may be just me.)

janezd · 2016-11-11T16:42:49Z

Orange/data/table.py

+
+        # class_var - class_name to class_var with type of class_type
+        self.Y, class_vars = np.empty((self.n_rows, 0)), []
+        if class_name:


I would prefer if class_name is not None unless you intentionally want to treat "" as false.

janezd · 2016-11-11T18:28:20Z

Orange/widgets/data/owtranspose.py

+        options = dict(callback=self.apply, orientation=Qt.Horizontal,
+                       labelWidth=100, contentsLength=12)
+        self.feature_model = itemmodels.VariableListModel()
+        self.feature_combo = gui.comboBox(


I don't like this second combo. Until I read the code I had no clue what it actually does. I don't find the idea that you can transpose the transpose and get the original appealing and very useful. (I know it wasn't your idea.)

janezd · 2016-11-11T18:31:18Z

Orange/widgets/data/owtranspose.py

+                list(a.attributes.keys()) for a in self.data.domain.attributes)
+            variables = chain.from_iterable(
+                (DiscreteVariable(name), ContinuousVariable(name))
+                for name in sorted(set(names)))


Yuck. I really don't like this transpose of transpose. This looks terrible, both in code as well as in GUI -- offering the same attribute twice, with the icon determining the type.

janezd · 2016-11-11T18:33:35Z

Orange/widgets/data/owtranspose.py

+                (DiscreteVariable(name), ContinuousVariable(name))
+                for name in sorted(set(names)))
+            self.class_model[:] += list(variables)
+        self.class_variable_index = min(2, len(self.class_model[:])) - 1


I find min(1, len(self.class_model[:]) - 1) more informative than using 2 since you're going to subtract 1. (Or have I misread it?)

Besides, you don't need [:]: VariableListModel defines __len__.

janezd · 2016-11-11T18:34:57Z

Orange/widgets/data/owtranspose.py

+            class_var = self.class_model[self.class_variable_index]
+            if class_var != "None":
+                options["class_name"] = class_var.name
+                options["class_type"] = "d" if class_var.is_discrete else "c"


If class_type argument was type, not str, you could use options["class_type"] = type(class_var) here.

janezd · 2016-11-11T19:43:04Z

Orange/widgets/data/owtranspose.py

+
+
+if __name__ == "__main__":
+    from PyQt4.QtGui import QApplication


ajdapretnar · 2016-11-18T09:58:18Z

Two comments:

The widget doesn't show class variable to retrieve feature names by, but it does the retrieval. In other words, iris-setosa appears in feature name, but it doesn't appear as a drop-down in the widget.
Fails on sparse data. Well, the widget itself doesn't fail, Data Table does, which makes sense, but I think a warning would be nice ('Can't transpose sparse data.')

Big thumbs up for the widget! 👍

janezd · 2016-11-25T11:27:59Z

Orange/widgets/data/owtranspose.py

+            box, self, "feature_names_column", orientation=Qt.Horizontal,
+            labelWidth=100, sendSelectedValue=True, callback=self.apply,
+            contentsLength=12, enabled=self.feature_type)
+        self.feature_combo.setModel(self.feature_model)


Indent the combo like this:

self.feature_combo = gui.comboBox( gui.indentedBox( box, gui.checkButtonOffsetHint(self.feature_radio.buttons[0])), self, "feature_names_column", orientation=Qt.Horizontal, labelWidth=100, sendSelectedValue=True, callback=self.apply, contentsLength=12, enabled=self.feature_type)

janezd · 2016-11-25T11:29:00Z

Orange/widgets/data/owtranspose.py

+            options = dict()
+            if self.feature_type:
+                options["feature_names_column"] = self.feature_names_column
+


Feel free to remove this empty line.

janezd · 2016-11-25T11:29:41Z

Orange/widgets/data/owtranspose.py

+    resizing_enabled = False
+    want_main_area = False
+
+    settingsHandler = PerfectDomainContextHandler(metas_in_res=True)


Why PerfectDomainContextHandler? I prefer DomainContextHandler when there is no need to match all attributes, because it matches more domains.

janezd · 2016-11-25T11:30:46Z

Orange/widgets/data/owtranspose.py

+
+    settingsHandler = PerfectDomainContextHandler(metas_in_res=True)
+    feature_type = ContextSetting(0)
+    feature_names_column = ContextSetting("")


This is Variable, not str. You should use None as default.

janezd · 2016-11-25T11:34:52Z

Orange/widgets/data/owtranspose.py

+        box = gui.vBox(self.controlArea, "Feature names")
+        self.feature_radio = gui.radioButtonsInBox(
+            box, self, "feature_type", callback=self._feature_type_changed,
+            btnLabels=["Generic", "From meta attributes"])


"From meta attribute: " (singular, colon)

janezd

While trying to find the reason for the bug described below, I fixed it so I just added a commit to your PR. Please review and verify. Also, please fix my mistake at the bottom.

janezd · 2016-11-25T20:10:33Z

Orange/widgets/data/owtranspose.py

            gui.indentedBox(
                box, gui.checkButtonOffsetHint(self.feature_radio.buttons[0])),
            self, "feature_names_column", callback=self._feature_combo_changed,
-            model=self.feature_model, sendSelectedValue=True,


sendSelectedValue is inapplicable when the combo uses a model --- which always returns the object from the model.

janezd · 2016-11-25T20:10:47Z

Orange/widgets/data/owtranspose.py

                box, gui.checkButtonOffsetHint(self.feature_radio.buttons[0])),
            self, "feature_names_column", callback=self._feature_combo_changed,
-            model=self.feature_model, sendSelectedValue=True,
-            labelWidth=100, contentsLength=12, orientation=Qt.Horizontal)


You don't need this since there is no label.

janezd · 2016-11-25T20:16:43Z

Orange/widgets/data/owtranspose.py


    def set_data(self, data):
-        self.closeContext()
+        # Skip the context if the combo is empty: a context with


I spent an hour hunting this down. The problem is that if you take Iris, it sets feature_names_column to None. The you take zoo. The Iris' context matches for zoo domain since it doesn't require any attribute - hence it will set feature_names_column to None. For the bug to be harder to track, the combo will not reflect this since it does not contain a None value.

This is not a bug in DomainContextHandler, but it will appear in all such cases. @astaric, what about preventing a match when no attribute were matched since no matcher were required?

PerfectDomainContextHandler would work here. :) But I still think that widgets shouldn't use PerfectDomainContextHandler unless unavoidable.

This could be the reason I used it in a first place.

janezd · 2016-11-25T20:17:10Z

Orange/widgets/data/owtranspose.py

        self.feature_model.set_domain(None)
        if self.data:
            self.feature_model.set_domain(self.data.domain)
-            if len(self.feature_model):


PyListModel has __bool__ defined.

janezd · 2016-11-25T20:17:39Z

Orange/widgets/data/owtranspose.py

        if self.data:
            self.feature_model.set_domain(self.data.domain)
-            if len(self.feature_model):
-                _names = [m.name for m in self.data.domain.metas if m.is_string]


The beauty of the domain model is that you don't need to do this. Besides, it should use m, not m.name.

janezd · 2016-11-25T20:17:54Z

Orange/widgets/data/owtranspose.py

-            if len(self.feature_model):
-                _names = [m.name for m in self.data.domain.metas if m.is_string]
-                self.feature_names_column = _names[0]
-        enabled = bool(len(self.feature_model))


Again, bool suffices here.

janezd · 2016-11-25T20:19:47Z

Orange/widgets/data/owtranspose.py

            try:
-                transposed = Table.transpose(self.data, **options)
+                transposed = Table.transpose(
+                    self.data, feature_names_column=self.feature_names_column)


This is a bit wrong, but I wouldn't like to push -f over it, and lose the comments I made above. Please change this to .... feature_names_column = self.feature_type and self.feature_names_column.

janezd · 2016-11-25T20:20:46Z

Orange/widgets/data/owtranspose.py


    app = QApplication([])
    ow = OWTranspose()
+    d = Table("iris")


This is the case where it doesn't work. (To see this, add print(self.feature_names_column) after self.openContext().)

janezd · 2016-11-25T20:26:19Z

The comments above are meaningless unless you read them in a diff, here https://github.com/biolab/orange3/pull/1738/files/8576ec1dd4675a04bcff0e1d4e1dda48116aff1b..99b80e860952d38d2ad2c9e8f4840adbc3e145cc.

janezd · 2016-11-28T18:43:39Z

Orange/widgets/data/owtranspose.py

+        # GUI
+        box = gui.vBox(self.controlArea, "Feature names")
+        self.feature_radio = gui.radioButtonsInBox(
+            box, self, "feature_type", callback=self.apply,


One more thing (sorry!): changing the radio button always applies even if the auto-apply is off. Change callback=self.apply to callback=lambda: self.apply().

(The problem is that callback is bound to the original self.apply before gui.auto_commit replaces it with the conditional apply.)

janezd · 2016-11-28T21:24:54Z

Icon contains invisible lines to the left, and the file is rather long. The picture contains two smaller tables; larger, less complex symbols work better, imho. Please consider the icon in the attachment.

Transpose.svg.zip

janezd · 2016-11-28T21:38:53Z

I added a commit with a fix for auto apply, and took the liberty to change the icon.

VesnaT force-pushed the transpose branch 2 times, most recently from 759671a to 8c2c6e9 Compare November 10, 2016 11:33

janezd reviewed Nov 11, 2016

View reviewed changes

VesnaT force-pushed the transpose branch 2 times, most recently from 4e4a14a to 9b71ba4 Compare November 15, 2016 14:55

kernc mentioned this pull request Nov 21, 2016

Provide the same filtering options in "Select Columns" widget as in "Select Rows" #1765

Closed

VesnaT changed the title ~~[ENH] OWTranspose: Add a new widget~~ [WIP][ENH] OWTranspose: Add a new widget Nov 24, 2016

VesnaT force-pushed the transpose branch from 9b71ba4 to 3dc0726 Compare November 25, 2016 09:17

janezd requested changes Nov 25, 2016

View reviewed changes

VesnaT added 2 commits November 25, 2016 14:59

Table: Add transpose method

d45f507

OWTranspose: Add a new widget

8576ec1

VesnaT force-pushed the transpose branch from 3dc0726 to 8576ec1 Compare November 25, 2016 14:28

VesnaT changed the title ~~[WIP][ENH] OWTranspose: Add a new widget~~ [ENH] OWTranspose: Add a new widget Nov 25, 2016

janezd reviewed Nov 25, 2016

View reviewed changes

VesnaT force-pushed the transpose branch from 99b80e8 to fc5edc5 Compare November 28, 2016 09:46

OWTranspose: Fix contexts, other minor fixes

bad6ed1

VesnaT force-pushed the transpose branch from fc5edc5 to bad6ed1 Compare November 28, 2016 10:46

janezd reviewed Nov 28, 2016

View reviewed changes

janezd added 2 commits November 28, 2016 22:30

Transpose: Fix auto apply

75eee27

Transpose: Change icon

bc7b988

janezd approved these changes Nov 28, 2016

View reviewed changes

janezd merged commit f3f5ca0 into biolab:master Nov 28, 2016



		if __name__ == "__main__":
		from PyQt4.QtGui import QApplication

Uh oh!

Conversation

VesnaT commented Nov 10, 2016 • 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 Nov 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 88.92% (diff: 100%)

Uh oh!

janezd left a comment

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!

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!

ajdapretnar commented Nov 18, 2016

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!

janezd left a comment

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!

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 commented Nov 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janezd commented Nov 28, 2016

Uh oh!

janezd commented Nov 28, 2016

Uh oh!

Reviewers

Assignees

Labels

VesnaT commented Nov 10, 2016 •

edited

Loading

codecov-io commented Nov 10, 2016 •

edited

Loading

janezd commented Nov 25, 2016 •

edited

Loading