[ENH] Domain Model: order without separators by jerneju · Pull Request #2697 · biolab/orange3

jerneju · 2017-10-20T12:25:56Z

Issue

Cannot set order and skip separators

Description of changes

Includes

Code changes
Tests
Documentation

janezd

I don't think this PR is useful.

I somebody wants to have a list without separators, he would, with this PR, call DomainModel(order=SEPARATED, separators=False). Why would one use an order with separator and then override separators? Wouldn't it make more sense to define an unseparated order instead of adding yet another argument to the function?

As for the second commit: can you justify when would one want to redefine the order? I believe that every widget has a natural order (or, if it doesn't, the order doesn't matter). Changing it would only confuse the user. Or is there a situation where you would need it?

If one changes the order when the domain is already set, he has to set the domain again? Changing the order does not make sense then. What the second commit is trying to achieve is equivalent to constructing a new model and calling setModel on a view.

In summary, the first commit could be replaced by adding an order without separators, while the second can be removed since setting a new model is a simpler and cleaner option.

janezd · 2017-10-20T12:33:41Z

                 valid_types=None, alphabetical=False, skip_hidden_vars=True, **kwargs):
+        """
+
+        :param order: How attributes, metas and classes are ordered and where between


We lately prefer Napoleon.

How attributes (...) -> Order of attributes, metas, classes, separators and other options

janezd · 2017-10-20T12:36:27Z

+
+        model = DomainModel(order=DomainModel.SEPARATED, separators=False)
+        model.set_domain(Domain(attrs, classes, metas))
+        self.assertEqual(list(model), classes + metas + attrs)


What about checking whether it works with separators=True?

I actually added this test before you responded.

janezd · 2017-10-20T12:41:49Z

+
+        :param order: How attributes, metas and classes are ordered and where between
+                      them are separators.
+        :param separators: Separators are ignored when set to False despite being set in order.


Separators are ignored -> Ignore separators.
When set to False -> When what if set to False?

I tried to reformulate the sentence, but couldn't. I believe it is because I disagree with the commit as a whole.

janezd · 2017-10-20T12:44:22Z

+        :param order: How attributes, metas and classes are ordered and where between
+                      them are separators.
+        :param separators: Separators are ignored when set to False despite being set in order.
+        :param placeholder: The first element.


Wrong. Placeholder is the text that is shown when no variable is selected (i.e., None). It is usually, but not necessarily, the first element.

See: https://github.com/jerneju/orange3/blob/e62c7a6b785dab00f5c619c561d4cb361636bce0/Orange/widgets/utils/itemmodels.py#L888. None is inserted at the beginning of order only if it's not already there.

janezd · 2017-10-20T12:44:43Z

+                      them are separators.
+        :param separators: Separators are ignored when set to False despite being set in order.
+        :param placeholder: The first element.
+        :param valid_types: For instance continuous, discrete, etc. variables.


"For instance" is an example, not a description.

janezd · 2017-10-20T12:46:30Z

+        :param separators: Separators are ignored when set to False despite being set in order.
+        :param placeholder: The first element.
+        :param valid_types: For instance continuous, discrete, etc. variables.
+        :param alphabetical: Alphabetical order of attributes or as they are ordered in domain.


If true, variables are sorted alphabetically.

janezd · 2017-10-20T12:47:04Z

+        :param placeholder: The first element.
+        :param valid_types: For instance continuous, discrete, etc. variables.
+        :param alphabetical: Alphabetical order of attributes or as they are ordered in domain.
+        :param skip_hidden_vars: hidden vars can be hidden or shown


This is not a description of the option. It also does not explain what are hidden vars.

janezd · 2017-10-20T12:47:13Z

+        :param valid_types: For instance continuous, discrete, etc. variables.
+        :param alphabetical: Alphabetical order of attributes or as they are ordered in domain.
+        :param skip_hidden_vars: hidden vars can be hidden or shown
+        :param kwargs:


jerneju · 2017-10-20T13:32:34Z

I removed the second commit very soon after opening this PR. @janezd sorry for wasting your time.

astaric · 2017-10-20T13:39:17Z

@janezd The idea would be to keep "default order", and skip separators.

When you override order, I agree, it does not make sense to skip separators, as you would not include them anyway. But we would prefer to have as many widgets as possible use the default order, which should include separators where they can be displayed and not where they cannot.

janezd · 2017-10-20T13:57:12Z

What is better? DomainModel(order=DomainModel.SEPARATED, separators=False) or DomainModel(order=DomainModel.UNSEPARATED)? Or is your point that we could use DomainModel(separator=False)?

If the latter, I'd prefer

SEPARATED = <as it is now>
UNSEPARATED = <same, without separators>

def __init__(self, order=None, separators=True, placeholder=None,
               valid_types=None, alphabetical=False, skip_hidden_vars=True, **kwargs):		                   valid_types=None, alphabetical=False, skip_hidden_vars=True, **kwargs):

    ...
    if order is None:
        order = self.SEPARATED is separators else self.UNSEPARATED

(and no further changes in the code) since this implementation corresponds to what the change (if I understand it correctly) is actually about: the default value for order depends on whether separators can be shown or not, and it also matches

When you override order, I agree, it does not make sense to skip separators.

Or, if you think it's better, we keep order=SEPARATED as default, but I'd maybe suggest implementing the change by

if not separators:
    order = [element for element in order if element is not self.SEPARATOR]

It's a bit longer, but more explicit.

codecov-io · 2017-10-20T14:27:01Z

Codecov Report

Merging #2697 into master will increase coverage by 0.2%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #2697     +/-   ##
=========================================
+ Coverage   75.83%   76.03%   +0.2%     
=========================================
  Files         338      338             
  Lines       59545    59627     +82     
=========================================
+ Hits        45156    45340    +184     
+ Misses      14389    14287    -102

astaric · 2017-10-20T18:18:12Z

It was the latter.

I would prefer to keep one default order so I would go with

if not separators:
    order = [element for element in order if element is not self.SEPARATOR]

janezd · 2017-10-20T23:49:07Z

This is just computing the same thing every time, instead of having it precomputed and stored as a class attribute. There actually are two different defaults, it's just the question of where do you compute the second one.

jerneju · 2017-10-23T08:54:14Z

Ok, I changed it as you two suggested.

janezd · 2017-10-27T20:26:54Z

    PRIMITIVE = (DiscreteVariable, ContinuousVariable)

-    def __init__(self, order=SEPARATED, placeholder=None,
+    def __init__(self, order=SEPARATED, separators=True, placeholder=None,


In general, beware of changing the parameter order or inserting arguments within the existing arguments. If anybody constructed a DomainModel by calling DomainModel(DomainModel.Primitive, "(No color)") and not DomainModel(DomainModel.Primitive, placeholder="(No color)"), this change would break his/her code.

However, we seem to have always given these arguments with keywords.

We could also add a *, before them to make it explicit that they can only be used as keyword arguments.

I like it. Not just here.

It makes it easier to manipulate arguments without breaking backward compatibility.

It forces us to name arguments in calls, which makes the code more readable and prevents making some mistakes we've made in the past.

We should at least encourage adding a * in new functions.

jerneju changed the title ~~[ENH] Domain Model: order without separators, change order~~ [ENH] Domain Model: order without separators Oct 20, 2017

janezd requested changes Oct 20, 2017

View reviewed changes

[ENH] Domain Model: hide separators

9425a1c

DomainModel: improve docstring

3ac3052

janezd approved these changes Oct 27, 2017

View reviewed changes

janezd reviewed Oct 27, 2017

View reviewed changes

janezd merged commit 0f8fb93 into biolab:master Oct 27, 2017

astaric added this to the 3.8 milestone Oct 28, 2017

jerneju deleted the domainmodel-changeorder branch November 3, 2017 08:31

Uh oh!

Conversation

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

Uh oh!

Issue

Description of changes

Includes

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!

jerneju commented Oct 20, 2017

Uh oh!

astaric commented Oct 20, 2017

Uh oh!

janezd commented Oct 20, 2017

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

astaric commented Oct 20, 2017

Uh oh!

janezd commented Oct 20, 2017

Uh oh!

jerneju commented Oct 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janezd Oct 28, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jerneju commented Oct 20, 2017 •

edited

Loading

codecov-io commented Oct 20, 2017 •

edited

Loading

janezd Oct 28, 2017 •

edited

Loading