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

Docstring return statements #789

Merged
merged 8 commits into from
May 18, 2023

Conversation

mauicv
Copy link
Collaborator

@mauicv mauicv commented May 15, 2023

what is this

Some return statements in alibi detect are formatted incorrectly. This PR fixes this issue #759. These changes are all taken from #748 which will be closed. As an example of the kind of thing this fixes compare this to this.

  • Add Docstrings section from alibi CONTRIBUTING.md here
  • Check this behaviour

@codecov
Copy link

codecov bot commented May 15, 2023

Codecov Report

Merging #789 (e517b8c) into master (5a38fc2) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #789   +/-   ##
=======================================
  Coverage   81.45%   81.45%           
=======================================
  Files         154      154           
  Lines       10048    10048           
=======================================
  Hits         8185     8185           
  Misses       1863     1863           
Impacted Files Coverage Δ
alibi_detect/ad/adversarialae.py 63.39% <ø> (ø)
alibi_detect/ad/model_distillation.py 91.80% <ø> (ø)
alibi_detect/base.py 85.45% <ø> (ø)
alibi_detect/cd/base.py 90.97% <ø> (ø)
alibi_detect/cd/base_online.py 88.23% <ø> (ø)
alibi_detect/cd/classifier.py 100.00% <ø> (ø)
alibi_detect/cd/context_aware.py 97.43% <ø> (ø)
alibi_detect/cd/keops/learned_kernel.py 94.16% <ø> (ø)
alibi_detect/cd/keops/mmd.py 98.21% <ø> (ø)
alibi_detect/cd/learned_kernel.py 100.00% <ø> (ø)
... and 44 more

@@ -0,0 +1,3 @@
894744537dc64f729a44f64fbcb58e6b3b5dc1aa
Copy link
Member

Choose a reason for hiding this comment

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

Since we're squashing commits, there should only be one entry per PR. Can you check if it's the case that the hash of the first commit is used after the squash?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was actually wondering about this and meant to check how it works, and it seems the above does not. It seems that we'll have to do two separate PRs one with the changes and one with the relevant commits in .git-blame-ignore-revs.

Copy link
Member

@jklaise jklaise May 16, 2023

Choose a reason for hiding this comment

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

Or squash locally before merging so the PR only has one commit? Edit: on second though, you'd need to know the commit hash before it's created so probably not...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well I could squash the PR commits to one and then add another commit before merging into master instead of squashing?

@@ -198,7 +198,7 @@ def permed_lsdds(

Returns
-------
Vector of B LSDD estimates for each permutation, H_lam_inv which may have been inferred, and optionally
Vector of B LSDD estimates for each permutation, H_lam_inv which may have been inferred, and optionally \
Copy link
Member

Choose a reason for hiding this comment

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

Are these explicit line breaks actually needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Its the difference between this and this. The issues are covered here. I need to add the CONTRIBUTING.md docstring section from alibi to this PR as well btw

Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't realise this, good spot @mauicv !

@mauicv mauicv requested review from jklaise and ascillitoe May 16, 2023 15:15
Copy link
Member

@jklaise jklaise left a comment

Choose a reason for hiding this comment

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

LGTM!

```

- Returning an object which contains multiple attributes and each attribute is described individually.
In this case the attribute name is written between single back-ticks and the type, if provided, would be written in
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick:

and the type, if provided, would be written

Could we be a little more specific here? Not detailing types in the general case (since already done by sphinx-autodoc-typehints), but giving types when multiple objects are contained in another object (i.e. a dict), makes a lot of sense IMO. However, in the latter case, are we saying this should always be done? Or its optional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we're hinting that it's better to give types however it's not something we've done historically. I've opened an issue to change this across the codebase and then perhaps we can change the language here to be a little stronger.

Copy link
Contributor

@ascillitoe ascillitoe left a comment

Choose a reason for hiding this comment

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

One minor nitpick (or question even), but otherwise LGTM!

@mauicv mauicv merged commit cbbee6c into SeldonIO:master May 18, 2023
9 checks passed
@ascillitoe ascillitoe linked an issue Jun 12, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix Returns sections of some docstrings
3 participants