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

Automatically move _apply() and _call() doc to __call__ #48

Closed
kohr-h opened this issue Nov 19, 2015 · 15 comments
Closed

Automatically move _apply() and _call() doc to __call__ #48

kohr-h opened this issue Nov 19, 2015 · 15 comments

Comments

@kohr-h
Copy link
Member

kohr-h commented Nov 19, 2015

Let's try to find out if there is a good and predictable way to move documentation from _apply() and _call() of Operator implementations to the respective __call__() method so it appears in the right place in the final doc. In principle, a script would for each subclass of Operator

  • take the respective cls._apply.__doc__
  • copy it to cls.__call__.__doc__
  • modify the line out : something to out : something, optional
  • add the note that if out is provided as argument, it is returned
  • replace cls._apply.__doc__ and cls._call.__doc__ with a hint not to use the functions directly but rather call operator(x) or operator(x, out), respectively

The question is where to put this functionality. Maybe one could implement it in Operator.__new__()?

@adler-j
Copy link
Member

adler-j commented Nov 19, 2015

This is a very nice idea. I think it would go into _OperatorMeta.__new__.

Quite honestly this is also the last chance we have to somehow unify _apply and _call, it would be very much preferable imo, and would greatly simplify this.

@kohr-h
Copy link
Member Author

kohr-h commented Nov 19, 2015

This is a very nice idea. I think it would go into _OperatorMeta.__new__.

Rather into _OperatorMeta.__call__. Some research necessary here.

Quite honestly this is also the last chance we have to somehow unify _apply and _call, it would be very much preferable imo, and would greatly simplify this.

I agree. So I suppose that would be a _call() method, either without out argument (old _call()) or with (an optional) out argument (old _apply()) and use inspect to check what it is.

Actually, we could get rid of the metaclass altogether by that if I remember correctly.

@adler-j
Copy link
Member

adler-j commented Nov 19, 2015

We have three cases of operators

  • inplace
  • out of place
  • inplace + out of place

This if we have a _call(x, out) for example, this does not imply the existence of a inplace operation. Some thought needed.

@kohr-h
Copy link
Member Author

kohr-h commented Nov 19, 2015

These could be reflected in the arguments to _call()

  • one positional argument (x) --> in-place
  • two positional arguments (x, out) --> out-of-place
  • two positional arguments, second optional (x, out=None) --> either way, depending on out

kwargs would be passed through from __call__().

@adler-j
Copy link
Member

adler-j commented Nov 19, 2015

Nice idea, we would need to be VERY explicit about this then. Would we enforce the naming? If so preferably only on out.

@kohr-h
Copy link
Member Author

kohr-h commented Nov 19, 2015

If so, definitely only on out. I'll try an implementation today so we get this done.

@kohr-h
Copy link
Member Author

kohr-h commented Nov 19, 2015

And agreed, this would be super-explicit in the docstring, and we should add a section "How to implement and operator" in the online doc.

@kohr-h
Copy link
Member Author

kohr-h commented Nov 19, 2015

I'll make a new issue for the _call() change.

@adler-j
Copy link
Member

adler-j commented Feb 9, 2016

This is ancient, do we work on it?

@kohr-h
Copy link
Member Author

kohr-h commented Feb 9, 2016

There's still an ancient branch, but I'm not so sure how to do this reliably. I started some coding and found myself writing a docstring parser. Then I realized that it was no easy fix. Maybe it's better to just always document Operator._call officially so the documentation is exposed. We could simply add a hint in Operator.__call__ so users look up at the right place in the docs.

@adler-j
Copy link
Member

adler-j commented Feb 9, 2016

What happens to a See Also placed inside Operator.__call__? Ill do a test.

@adler-j
Copy link
Member

adler-j commented Feb 9, 2016

Seems it links to Operator._call, damnit.

@kohr-h
Copy link
Member Author

kohr-h commented Feb 9, 2016

That was my first thought, too, but having it point to the overridden _call automatically is not so easy I guess.

@kohr-h
Copy link
Member Author

kohr-h commented Mar 22, 2016

Any more thoughts here? I say we close this and instead encourage users to write a short description of the operator, roughly what it does, in the class docstring. Extensive explanations in the _call docstring are of limited use anyway.

@adler-j
Copy link
Member

adler-j commented Mar 22, 2016

I agree.

@adler-j adler-j closed this as completed Mar 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants