[ENH] Allow custom (generic) names in Transpose Widget #2737

jerneju · 2017-11-03T10:55:38Z

Issue

Fixes #2729

Description of changes

Includes

Code changes
Tests
Documentation

codecov-io · 2017-11-03T15:04:57Z

Codecov Report

Merging #2737 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2737      +/-   ##
==========================================
+ Coverage    76.1%   76.14%   +0.03%     
==========================================
  Files         338      338              
  Lines       59777    59839      +62     
==========================================
+ Hits        45496    45566      +70     
+ Misses      14281    14273       -8

janezd · 2017-11-10T14:46:08Z

Not like this. Keep two radio buttons, Generic and From meta attribute. Generic than has a line edit with placeholder Feature.

Indent the line edit so that it is aligned with the combo.

janezd · 2017-11-20T10:16:30Z

Orange/widgets/data/owtranspose.py

@@ -7,6 +11,28 @@
 from Orange.widgets.widget import Input, Output


+def add_radio_buttons(widget, master, value, btn_labels=(), orientation=Qt.Vertical, callback=None):


I don't like this function at all. It is called add_radio_buttons, and it appends radio buttons and line edits. And then a combo is added separately since this function handles only line edits. (Well, if it handled everything, it would just replace any placing of components inside control area.)

It looks general, but is called only once and replace what we would otherwise do with a sequence of calls to gui method that we usually use for constructing the interface. I would suggest refactoring this code into __init__.

I wanted to make it general, well then I moved everything into __init__.

janezd · 2017-11-20T17:26:29Z

Orange/widgets/data/owtranspose.py

+        self.feature_type = self.GENERIC
+        self.custom_feature_name.setText(self.custom_feature_name.text().strip())
+        self.feature_name = self.custom_feature_name.text()
+        if self.auto_apply:


Why if self.auto_apply? gui.auto_commit already replaced the apply method with one that applies only when outputs data if auto_apply is one.

The condition is not only redundant, it breaks the widget. Disable auto apply. Change the line edit. Enable auto apply. This should update the output but it doesn't since the dirty flag was not set because the apply function was not called when the auto apply was off.

If you added if to stop the changes from being applied when auto apply was disabled: the problem is that the auto apply button is a default button, hence Enter triggers it. To prevent this, use self.apply_button.button.setAutoDefault(False).

Exactly, that was the reason I added that if.

janezd · 2017-11-20T17:29:14Z

Orange/widgets/data/owtranspose.py

+    def _apply_editing(self):
+        self.feature_type = self.GENERIC
+        self.custom_feature_name.setText(self.custom_feature_name.text().strip())
+        self.feature_name = self.custom_feature_name.text()


These two lines get the text from the line edit, strip it, set it back to the line edit, and then read it from the line edit back to the widget's attribute.

self.feature_name = self.feature_name.strip() would do the whole job if gui.lineedit was used.

janezd · 2017-11-20T17:44:59Z

Orange/widgets/data/owtranspose.py

+        bg.group = rb
+        bg.buttons = []
+        bg.ogValue = value = "feature_type"
+        bg.ogMaster = self


This code comes from gui.radioButtons! Why copying it instead of using it?

janezd · 2017-11-20T17:46:13Z

Orange/widgets/data/owtranspose.py

+        bg.ogValue = value = "feature_type"
+        bg.ogMaster = self
+
+        for i, label in enumerate(["Generic", edit, "From meta attribute:"]):


This loop contains an if. The first case happens twice. It could be replaced with a single line (instead of two here -- the condition and the line). The elif part happens once. Get rid of the loop and the if's. The code will be two lines shorter and way more readable.

janezd · 2017-11-20T17:46:28Z

Orange/widgets/data/owtranspose.py

-            box, self, "feature_type", callback=lambda: self.apply(),
-            btnLabels=["Generic", "From meta attribute:"])
+
+        self.custom_feature_name = edit = QLineEdit()


Why not gui.lineEdit?! It would simplify the code; that's why we have gui.

janezd · 2017-11-20T17:47:25Z

Orange/widgets/data/owtranspose.py

+
+    def _get_feature_name(self):
+        edit = self.custom_feature_name
+        return edit.text().strip() or edit.placeholderText()


I'd prefer having the generic text as a constant instead of copying from the placeholder.

janezd · 2017-11-20T17:49:10Z

Orange/widgets/data/owtranspose.py


    def apply(self):
        self.clear_messages()
        transposed = None
+        kwargs = {} if self.feature_type != self.GENERIC else \
+            {"feature_name": self._get_feature_name()}


Just give the feature_name argument, even if the method won't use it. Better than this hack.

janezd · 2017-11-20T17:53:44Z

Orange/widgets/data/owtranspose.py

-        self.feature_type = int(enabled)
+        self.feature_type = self.FROM_META_ATTR if enabled else self.GENERIC
+
+    def _apply_editing(self):


Put this function before _feature_combo_changed; they belong together since they handle the same type of event.

janezd · 2017-11-20T18:06:36Z

Orange/widgets/data/owtranspose.py

@@ -76,25 +106,40 @@ def update_controls(self):
            if self.feature_model:
                self.feature_names_column = self.feature_model[0]
        enabled = bool(self.feature_model)
-        self.feature_radio.buttons[1].setEnabled(enabled)
+        self.feature_radio.buttons[self.FROM_META_ATTR].setEnabled(enabled)


The previous version allowed changing the radio button, but prevented changing the combo when no data. I don't see much point in disabling the combo and lineedit, and leave the radios on.

On the other hand, is there any harm in letting the user monkey around the controls when there is no data? I'd just leave them enabled.

janezd · 2017-11-20T19:53:06Z

I pushed my take on it.

janezd · 2017-11-20T20:32:53Z

Orange/widgets/data/tests/test_owtranspose.py

+    def test_transpose_custom_name(self):
+        """
+        Custom name can be set.
+        GH-2737


In my opinion -- and I would want to hear what @astaric, @lanzagar and @kernc think about it -- tests should test some functionality and not specific bugs that were discovered, or be related to some specific issue in any way. Hence, I do not like comments like GH-2737. This makes our unit tests look like a bunch of tests for patches. This is irrelevant -- potential bugs must be covered by "normal" tests, but bug-specific tests.

This bothered me for some time already, but now I stumbled upon this again when I wanted to reorganize tests for this widget, and I decided to "dissolve" this test into some other test.

Again, this is just my opinion.

but bug-specific tests. -> not bug specific tests.

Fixing bugs or commonplace stuff like test_report, sure. But in general, it's not necessarily obvious why a certain feature was added in amendments as opposed to initially. It's customary to cross reference the issue(s) where/when the surrounding discussion occurred. Case by case; ymmv.

janezd · 2017-11-20T20:33:10Z

Orange/widgets/data/tests/test_owtranspose.py

+
+    def test_send_report(self):
+        """
+        GH-2773


Here is an even better example: test_send_report should be a test for reports. Why is it marked as "GH-2737"?

lanzagar · 2017-11-27T13:29:07Z

Orange/widgets/data/owtranspose.py

+        edit = gui.lineEdit(
+            gui.indentedBox(box, gui.checkButtonOffsetHint(button)), self,
+            "feature_name",
+            placeholderText="Prefix for column names",


Currently, I can not read the whole placeholder text because the widget is too small and not resizable.
We can leave the text as is and just make the widget a bit larger?

lanzagar · 2017-11-27T13:36:43Z

Orange/widgets/data/tests/test_owtranspose.py

+        widget.apply()
+        output = self.get_output(widget.Outputs.data)
+        self.assertTrue(
+            all(x.name.startswith("Feature ")


Wouldn't it be better to test that names start with OWTranspose.DEFAULT_PREFIX?
(in case someone changes the default in the future)

lanzagar · 2017-11-27T13:36:48Z

Orange/widgets/data/tests/test_owtranspose.py

+        widget.apply()
+        output = self.get_output(widget.Outputs.data)
+        self.assertTrue(
+            all(x.name.startswith("Feature ")


Wouldn't it be better to test that names start with OWTranspose.DEFAULT_PREFIX?
(in case someone changes the default in the future)

jerneju changed the title ~~[FIX] Allow custom (generic) names in Transpose Widget~~ [WIP][ENH] Allow custom (generic) names in Transpose Widget Nov 3, 2017

jerneju changed the title ~~[WIP][ENH] Allow custom (generic) names in Transpose Widget~~ [ENH] Allow custom (generic) names in Transpose Widget Nov 10, 2017

janezd reviewed Nov 20, 2017

View reviewed changes

jerneju added this to the 3.8 milestone Nov 22, 2017

lanzagar reviewed Nov 27, 2017

View reviewed changes

Table transpose: allow custom feature names

ea250b8

lanzagar merged commit ed53796 into biolab:master Nov 27, 2017

jerneju deleted the transpose-custom-name branch November 27, 2017 15:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] Allow custom (generic) names in Transpose Widget #2737

[ENH] Allow custom (generic) names in Transpose Widget #2737

jerneju commented Nov 3, 2017 •

edited

Loading

codecov-io commented Nov 3, 2017 •

edited

Loading

janezd commented Nov 10, 2017

janezd Nov 20, 2017

jerneju Nov 20, 2017

janezd Nov 20, 2017

janezd Nov 20, 2017

jerneju Nov 21, 2017

janezd Nov 20, 2017

janezd Nov 20, 2017

janezd Nov 20, 2017

janezd Nov 20, 2017

janezd Nov 20, 2017

janezd Nov 20, 2017

janezd Nov 20, 2017

janezd Nov 20, 2017

janezd commented Nov 20, 2017 •

edited

Loading

janezd Nov 20, 2017

janezd Nov 20, 2017

kernc Nov 20, 2017 •

edited

Loading

janezd Nov 20, 2017

lanzagar Nov 27, 2017

lanzagar Nov 27, 2017

lanzagar Nov 27, 2017

		@@ -7,6 +11,28 @@
		from Orange.widgets.widget import Input, Output


		def add_radio_buttons(widget, master, value, btn_labels=(), orientation=Qt.Vertical, callback=None):

[ENH] Allow custom (generic) names in Transpose Widget #2737

[ENH] Allow custom (generic) names in Transpose Widget #2737

Conversation

jerneju commented Nov 3, 2017 • edited Loading

Issue

Description of changes

Includes

codecov-io commented Nov 3, 2017 • edited Loading

Codecov Report

janezd commented Nov 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janezd commented Nov 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kernc Nov 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerneju commented Nov 3, 2017 •

edited

Loading

codecov-io commented Nov 3, 2017 •

edited

Loading

janezd commented Nov 20, 2017 •

edited

Loading

kernc Nov 20, 2017 •

edited

Loading