Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MAINT: unify usage of calls to parent classes, closes #1061 #1161

Merged
merged 3 commits into from
Sep 26, 2017

Conversation

kohr-h
Copy link
Member

@kohr-h kohr-h commented Sep 26, 2017

Getting this out of the way.

Caveat

There are some cases when super() is the wrong choice, most notably with multiple inheritance and "conflicting" methods. Everything is good as long as a certain method only exists once or things stay nicely in order. However, consider the case of ScalingFunctional which I stumbled over while doing this change.

We have class ScalingFunctional(Functional, ScalingOperator), with MRO

[odl.solvers.functional.default_functionals.ScalingFunctional,
 odl.solvers.functional.functional.Functional,
 odl.operator.default_ops.ScalingOperator,
 odl.operator.operator.Operator,
 object]

So far, so good. The __init__ method of ScalingFunctional even hardcodes the __init__ calls to both parent classes, not using super. Everything should be fine, or?

Turns out, no. The crucial part is the common misconception (also on my part) that in an instance method of a class, self always refers to an instance of that class. That's not true. In this case, when Functional.__init__ uses self, then self is an instance of ScalingFunctional. In particular, when using super to resolve the MRO, it is still the one of ScalingFunctional.

Hence, when Functional.__init__ calls super(Functional, self).__init__(...), then the __init__ method that is called is not the one of Operator (the only parent of Functional), but the one of ScalingOperator, because it comes first in the MRO.
We can confirm this by printing self.__class__.mro() in Functional.__init__:

[<class 'odl.solvers.functional.default_functionals.ScalingFunctional'>,
 <class 'odl.solvers.functional.functional.Functional'>,
 <class 'odl.operator.default_ops.ScalingOperator'>,
 <class 'odl.operator.operator.Operator'>,
 <class 'object'>]

Conclusion

We have to be careful with multiple inheritance the MRO contains potentially conflicting methods. As the documentation says, super is intended for collaborative classes, and this situation is not like that.
Sometimes it's hard to tell who will call what. Two ways to mitigate this are:

  1. Don't use super in classes that may participate in non-collaborative multiple inheritance. That applies mostly to core classes. It may be hard to know in advance which classes will be affected.
  2. Avoid non-collaborative multiple inheritance as far as possible. In particular, try to keep the class hierarchy linear if possible.

Copy link
Member

@adler-j adler-j left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done, only some very minor stuff to comment on.

range=odl.rn(4), linear=True)
super(MyOp, self).__init__(
domain=ProductSpace(odl.rn(3),
ProductSpace(odl.rn(3), odl.rn(3))),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

odl.rn(3) * (odl.rn(3) * odl.rn(3))

linear=True)
super(MyOp, self).__init__(
domain=odl.rn(3),
range=ProductSpace(odl.rn(3),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

odl.rn(3) * (odl.rn(3) * odl.rn(3))

@kohr-h
Copy link
Member Author

kohr-h commented Sep 26, 2017

Rebased and fixed, merging after CI.

@kohr-h kohr-h merged commit b14b962 into odlgroup:master Sep 26, 2017
@kohr-h kohr-h deleted the issue-1061__super branch September 26, 2017 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants