Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Update docstrings to numpydoc format #2237

Merged
merged 6 commits into from
Feb 4, 2022

Conversation

dhomeier
Copy link
Collaborator

@dhomeier dhomeier commented Oct 1, 2021

Description

This is beginning to address #466; I have tried to clarify or extend the docs where they seemed ambiguous or incomplete, but someone more familiar with the functionality should review the added Parameters, Returns etc. entries.

Note that #2235 includes updates for roi.py since a lot of changes and extensions to the docstrings were already added there.

@astrofrog
Copy link
Member

@dhomeier - could you rebase? Sorry for not reviewing this sonner!

@codecov
Copy link

codecov bot commented Jan 29, 2022

Codecov Report

Merging #2237 (5604178) into main (55aca29) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2237   +/-   ##
=======================================
  Coverage   88.07%   88.07%           
=======================================
  Files         247      247           
  Lines       23279    23279           
=======================================
  Hits        20502    20502           
  Misses       2777     2777           
Impacted Files Coverage Δ
glue/core/application_base.py 82.53% <ø> (ø)
glue/core/command.py 93.85% <ø> (ø)
glue/core/component.py 95.85% <ø> (ø)
glue/core/component_id.py 89.24% <ø> (ø)
glue/core/component_link.py 86.48% <ø> (ø)
glue/core/coordinate_helpers.py 90.66% <ø> (ø)
glue/core/coordinates.py 90.62% <ø> (ø)
glue/core/data.py 91.66% <ø> (ø)
glue/core/data_collection.py 90.33% <ø> (ø)
glue/core/data_combo_helper.py 91.85% <ø> (ø)
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55aca29...5604178. Read the comment docs.

@dhomeier
Copy link
Collaborator Author

No problem! I did a cursory search for new function's docstrings added since, or any that I've missed before.

@dhomeier
Copy link
Collaborator Author

Beats me why the contracted form works in some cases like ~numpy.ndarray but just breaks the links in others.

Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

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

Looks good! However the docs failures are related to this PR, so could you investigate? (I think just some incorrect intersphinx references)

@dhomeier
Copy link
Collaborator Author

dhomeier commented Feb 3, 2022

Looks good! However the docs failures are related to this PR, so could you investigate? (I think just some incorrect intersphinx references)

Can you point me to an example where they are failing? I think the last build here was https://readthedocs.org/projects/glueviz/builds/15921389/

@astrofrog
Copy link
Member

@dhomeier - ah I meant the Azure build, e.g. the tox py37-docs-pyqt513 config

@dhomeier
Copy link
Collaborator Author

dhomeier commented Feb 3, 2022

Yeah, sorry, just found it – I had missed that among the other pyside + pyqt51x failures!

@dhomeier
Copy link
Collaborator Author

dhomeier commented Feb 3, 2022

Warning, treated as error:
/home/vsts/work/1/s/.tox/py37-docs-pyqt513/lib/python3.7/site-packages/glue/core/data.py:docstring of glue.core.data.BaseCartesianData.compute_fixed_resolution_buffer:14:py:class reference target not found: glue.core.Data

Not quite an intersphinx issue then – that reference was indeed broken, missing the full class path. Still a mystery to me why the plain ~glue.core.Data version did not raise (but the links in http://docs.glueviz.org/en/stable/api/glue.core.data.BaseCartesianData.html#glue.core.data.BaseCartesianData.compute_fixed_resolution_buffer are all dead, so maybe numpydoc never looked at them).

@astrofrog
Copy link
Member

@dhomeier - it looks like there might still be some issues as the Azure docs builds are still failing?

@dhomeier
Copy link
Collaborator Author

dhomeier commented Feb 3, 2022

Yes, I realised I need to sphinx-build -Wn as well to track them all down!

@dhomeier
Copy link
Collaborator Author

dhomeier commented Feb 4, 2022

Still a ton of warnings the likes of

docstring of glue.viewers.common.qt.toolbar.BasicToolbar.actionTriggered:2: WARNING: Inline emphasis start-string without end-string.

glue/viewers/common/qt/toolbar.py:docstring of glue.viewers.common.qt.toolbar.BasicToolbar.create:: WARNING: py:class reference target not found: PyQt5.sip.voidptr

As they are pretty much inherited from PyQt5.QtWidgets.QToolBar they seem a bit out of control; so I hope docs-test will let them pass through.

Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

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

Looks great, thanks! For the reference issues related to Qt I usually add exceptions in conf.py but you could always add that in a separate PR if you like

@astrofrog astrofrog merged commit 164424f into glue-viz:main Feb 4, 2022
@dhomeier
Copy link
Collaborator Author

dhomeier commented Feb 4, 2022

Thanks, yes, seems a ('py:class', 'PyQt5.sip.voidptr') would already have done it, but apparently the Azure builds don't care about that – and fortunately neither throw up the "Inline emphasis start-string without end-string" stuff, which is not a nitpick afaict.

@dhomeier dhomeier deleted the numpydoc-strings branch February 4, 2022 18:29
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants