Skip to content

Conversation

@adler-j
Copy link
Member

@adler-j adler-j commented Feb 22, 2017

So this PR is currently very small and is intended as something we can use for discussion.

The overall idea here is rather far reaching, but would (imo) make ODL easier to understand (on the surface, at least) and reduce the possibility of mistakes.

The idea is that we replace the current behavior of users implementing _call, _inner etc, with a more general interface wherein users implement the actual methods that they would want called __call__, inner, etc, and we wrap them by using a metaclass.

As an example, I have implemented this for the derivative, and I'm hoping for input on this. If we feel it is a good paradigm, we could implement this change and replace lots of the _ methods.

The main change for external users would be that they would be implementing the python standard methods instead of some ODL special thing, and also that stuff like

def derivative(x):
    x = self.domain.element(x)
    return 2 * x

could be simplified to

def derivative(x):
    return 2 * x

@kohr-h
Copy link
Member

kohr-h commented Feb 23, 2017

Interesting idea. I've been thinking myself why we have input checks and wrapping in __call__ but not in derivative, gradient, proximal and the likes. With such a change, of course, Functional would have to receive the same treatment.

I'm all for it (as long as ABCMeta is not involved :-) ).

@kohr-h
Copy link
Member

kohr-h commented Mar 3, 2017

The main change for external users would be that they would be implementing the python standard methods instead of some ODL special thing, and also that stuff like

I'm having second thoughts on this. The metaclass approach, while convenient and simple to maintain, lifts everything a good deal farther into the "magical" regime. In some sense, the _call, _inner etc. methods are at least "honest" and don't pretend to implement the interface. They're more open on the fact that they are mere implementations.

Here's my pros and cons list:

Underscore methods Metaclass
Interface A bit clunky 👎 Nice 👍
Resulting code Concise, since assumptions can be made; however, users don't seem to get it at first ❓ Concise, but users are more likely to miss the magic behavior and do unnecessary work ❓
Maintainability Changing a method to this pattern leads to API change 👎 Changes are only in the metaclass 👍
Surprise factor Small, since called method != implemented method 👍 Large, since called method == implemented method, input & output transformation is totally magic 👎

Seems like there are still more 👍's on the right side of the table...

@adler-j adler-j mentioned this pull request Jul 3, 2017
20 tasks
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