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

Update docstrings to be consistent with function signature #3310

Merged
merged 7 commits into from May 29, 2021

Conversation

cphyc
Copy link
Member

@cphyc cphyc commented May 28, 2021

PR Summary

With time, many docstrings have grown out of sync with their actual function. This PR attemps to fix as many as possible.

This PR fixes as many as possible:

  • Non-compliant docstrings (e.g. * ds : Dataset should be ds : Dataset)
  • Arguments mentioned in the docstring that do not exist in the function signature
  • Plot callbacks and fixed resolution buffer filters had their signatures incorrectly guessed (see screenshots). Note that the best solution would be to refactor the callbacks and remove the current two hacks to get the proper function signature. See issue ENH: Refactor annotation callbacks #3311 .

The mismatches have been found using https://gist.github.com/cphyc/afc1a143943220ec3a1c5ca16e14cd13 which allows to compare the list of argument parsed from the docstring to the actual signature of the function.

Currently on main

import yt
ds = yt.load_sample("output_00080")
p = yt.SlicePlot(ds, "x", "density")
p.annotate_sphere?

On main (center is missing from the signature):
image

With this PR:
image

Note on the signature issue

The origin of the signature mismatch is due to the weird structure we are using to register plot annotation callbacks. Each callback is a class which constructor we were binding to the caller PlotWindow instance using types.MethodType. This function alters the displayed signature by dropping the first argument which is assumed to be self. In our case, because we are binding a constructor method rather than a function, the first argument in the signature is actually the first argument after self in the __init__ method.

Here's a simple example:

>>> import types
>>> class Foo:
...     def __init__(self, arg1, arg2):
...         pass
...         
... class Bar:
...     def __init__(self):
...          self.foo = types.MethodType(Foo, self)
... 
... b = Bar()
... b.foo?
Signature: b.foo(arg2)   # ! Should have been b.foo(arg1, arg2)
Docstring: <no docstring>
Type:      method

@cphyc cphyc changed the title Remove non-existing parameters Update docstrings to be consistent with function signature May 28, 2021
yt/loaders.py Outdated
Comment on lines 1068 to 1070
length_unit=None,
bbox=None,
sim_time=0.0,
length_unit=None,
Copy link
Member

Choose a reason for hiding this comment

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

If you're worried about order consistency, please sort the docstring, not the signature itself. Specifically positional arguments (keyword-only arguments can be safely reordered without breaking backwards compatibility however)

Copy link
Member

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

thank's for doing this, here are a couple suggestions

yt/visualization/color_maps.py Outdated Show resolved Hide resolved
yt/visualization/fits_image.py Outdated Show resolved Hide resolved
@@ -222,7 +222,7 @@ def from_lines(
figure_size : int or two-element iterable of ints
Size in inches of the image.
Default: 5 (5x5)
fontsize : int
font_size : int
Copy link
Member

Choose a reason for hiding this comment

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

I agree with the annotated type, but the default value's type should also reflect it, can you fix it too ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be good now!

Co-authored-by: Clément Robert <cr52@protonmail.com>
@@ -222,7 +222,7 @@ def from_lines(
figure_size : int or two-element iterable of ints
Size in inches of the image.
Default: 5 (5x5)
fontsize : int
font_size : int
Copy link
Member Author

Choose a reason for hiding this comment

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

Should be good now!

filt.__doc__ = FilterMaker.__doc__
self.__dict__["apply_" + filtername] = types.MethodType(filt, self)

# We need to wrap to create a closure so that
Copy link
Member Author

Choose a reason for hiding this comment

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

I have updated the PR description to explain why this is required.

@cphyc cphyc added api-consistency naming conventions, code deduplication, informative error messages, code smells... bug docs yt-4.0 feature targeted for the yt-4.0 release and removed yt-4.0 feature targeted for the yt-4.0 release labels May 28, 2021
@cphyc cphyc added the yt-4.0 feature targeted for the yt-4.0 release label May 28, 2021
Copy link
Member

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

I'm happy to have this go in as is given the fact we have a freshly opened issue to remind us that this code is due for a deeper refactor. Thanks again !

@neutrinoceros neutrinoceros merged commit d7b1b99 into yt-project:main May 29, 2021
@cphyc cphyc deleted the pydoctest branch October 19, 2021 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-consistency naming conventions, code deduplication, informative error messages, code smells... bug docs yt-4.0 feature targeted for the yt-4.0 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants