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

__call__ explained with example #5390

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

hiren-2911
Copy link

Description

Checklist

For reviewers

  • Check that the PR title is short, concise, and will make sense 1 year
    later.
  • Check that new functions are imported in corresponding __init__.py.
  • Check that new features, API changes, and deprecations are mentioned in
    doc/release/release_dev.rst.

@pep8speaks
Copy link

pep8speaks commented May 12, 2021

Hello @hiren-2911! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 624:1: W293 blank line contains whitespace

Comment last updated at 2021-05-20 07:34:17 UTC

@mkcor
Copy link
Member

mkcor commented May 12, 2021

Hello @hiren-2911,

Thank you for your interest in the library! It looks like the description you've added is copied-pasted from popular Python courses. It's generic, not specific to geometric transformations. What is your intention with this contribution?

To include examples in a docstring, please refer these docs for the syntax to use.

Thanks again,
Marianne

@hiren-2911
Copy link
Author

I am really sorry for my previous commits, I am contributing to open source for the first time and I was totally unaware.
Moreover I misunderstood the issue and gave a generic example instead of proving the legit example.

@alexdesiqueira alexdesiqueira linked an issue May 16, 2021 that may be closed by this pull request
@alexdesiqueira
Copy link
Member

Hello @hiren-2911,
thank you for taking the time to tackle #5309!
Would you like to check _geometric.py? It can help you when designing that example.
Thanks again!

@mjmdavis
Copy link

Perhaps the class comment could say:

"""
...
Instances of ProjectiveTransform are callable.
"""

[7 8 9]].

A=ProjectiveTransform(matrix) #instance created.
B=A(coords) #calling instance A as a function
Copy link
Member

@hmaarrfk hmaarrfk May 18, 2021

Choose a reason for hiding this comment

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

Suggested change
B=A(coords) #calling instance A as a function
>>> B = A(coords) # calling instance A as a function
>>> print(B)

can you provide the output of B?

Copy link
Member

Choose a reason for hiding this comment

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

Unresolving conversation because I don't see the output of B anywhere...

Vaghela Hiren and others added 3 commits May 20, 2021 13:00
Co-authored-by: Mark Harfouche <mark.harfouche@gmail.com>
Co-authored-by: Mark Harfouche <mark.harfouche@gmail.com>
Co-authored-by: Mark Harfouche <mark.harfouche@gmail.com>
Comment on lines +618 to +619
to behave like functions. The user can call
these instances like functions.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
to behave like functions. The user can call
these instances like functions.
to behave like a function.

Formatting-wise, I don't think lines need to be that short. Content-wise, have you seen the comments by @alexdesiqueira and @mjmdavis?

... [7, 8, 9]]

>>> A = ProjectiveTransform(matrix) # instance created.
>>> B = A(coords) # calling instance A as a function
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
>>> B = A(coords) # calling instance A as a function
>>> B = A(coords) # call instance A as a function

... [4, 5, 6],
... [7, 8, 9]]

>>> A = ProjectiveTransform(matrix) # instance created.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
>>> A = ProjectiveTransform(matrix) # instance created.
>>> A = ProjectiveTransform(matrix) # create instance

(for the sake of consistency)

>>> A = ProjectiveTransform(matrix) # instance created.
>>> B = A(coords) # calling instance A as a function
>>> print(B)
with "coords" as argument.
Copy link
Member

Choose a reason for hiding this comment

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

What is this line doing out here?

@grlee77 grlee77 added the 📄 type: Documentation Updates, fixes and additions to documentation label Aug 20, 2021
@lagru
Copy link
Member

lagru commented Mar 10, 2023

Hey @hiren-2911, thank you for the contribution so far! Do you intend to and are you able to continue working on this?

@lagru lagru added the ⚽ Contributor's ball Waiting for action from the contributor label Mar 10, 2023
@hiren-2911
Copy link
Author

hiren-2911 commented Mar 10, 2023 via email

@lagru
Copy link
Member

lagru commented Mar 10, 2023

Additionally, it doesn't look like the docstring of methods such as __call__ is rendered in our current HTML (see rendering of ProjectiveTransform). I'd propose to move the example section to the class docstring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚽ Contributor's ball Waiting for action from the contributor 📄 type: Documentation Updates, fixes and additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

__call__ unclear in documentation
8 participants