Misc style #1135

Merged
merged 5 commits into from Oct 23, 2013

Projects

None yet

4 participants

@j-grana6
Contributor

Will replace PR 959

@j-grana6 j-grana6 referenced this pull request Oct 22, 2013
Closed

Pylintedits #959

@coveralls

Coverage Status

Coverage remained the same when pulling f7060c4 on j-grana6:misc_style into 3b7082c on statsmodels:master.

@josef-pkt
Member

Looks good, I don't see any problems merging this.
It's mostly cleanup that doesn't change the behavior and can go into 0.5.1 or not.

it's still not clear to me whether the _OptFuncts classes should be Mixin classes and the empty __init__ deleted nor what the base class with non-empty __init__ should be.

In general: If we only inherit from a mixin class, do we need to inherit from (object, Mixin) or just (Mixin)? I never tried.

@j-grana6
Contributor

On 10/22/2013 09:01 PM, Josef Perktold wrote:

Looks good, I don't see any problems merging this.
It's mostly cleanup that doesn't change the behavior and can go into
0.5.1 or not.

it's still not clear to me whether the |_OptFuncts| classes should be
Mixin classes and the empty |init| deleted nor what the base class
with non-empty |init| should be.

In general: If we only inherit from a mixin class, do we need to
inherit from |(object, Mixin)| or just |(Mixin)|? I never tried.

Mixin are new to me so I'm not sure if the question is on the
logical/philosophical level or the actual functionality. However of the
many explanations I found this one seemed to highlight the ambiguity in
this case :

"(1) Often a mix-in is not the “primary” superclass of any given class,
(2) does not care what class it is used with, is used with many (3)
classes scattered throughout the class hierarchy and is (4) introduced
dynamically at runtime."

So, in this case, (1) is not true, (2) is true, (3) is somewhat true in
that it is scattered but it is at the same level in the hierarchy and
I'm not sure what (4) is.


Reply to this email directly or view it on GitHub
#1135 (comment).

@josef-pkt
Member

about Mixin classes

we haven't discussed them much yet, because we don't make much use of them, so far, in statsmodels. I started to add them to class names in the unit tests to make pylint stop complaining about attributes.

From a quick Google search it seems there is quite a wide range of classes that developers call Mixins.
We want to use in statsmodels a very restrictive definition of Mixins, so we don't make our life to complicated with multible inheritance.

  • may use attributes of self that it didn't define (makes pylint happy)
  • should not create it's own attributes that are reused by base class methods (may be a rule that allows exceptions)
  • has no __init__ method (follows largely from previous point)
  • should define new methods and stay out of the way of the regular inheritance chain and the super calls in those methods (this is mainly to avoid having to think a lot about inheritance order in super calls with multiple inheritance)
  • I'm not sure we want to have a inheritance hierarchy of Mixin classes, but it might be likely that we do.

The toplevel _OptFuncts classes don't define and store their own attributes, their __init__ is empty and they provide additional methods to the subclasses. So, maybe they are Mixins.

@jseabold jseabold merged commit bb0148b into statsmodels:master Oct 23, 2013

1 check passed

default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment