Tickets/dm-15684: Convert to numpydoc #106
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
isullivan
force-pushed
the
tickets/DM-15684
branch
2 times, most recently
from
9b3a7ea
to
e75520f
Compare
|
@isullivan Thanks for picking this up!. Just as a pre-review thing, there are a bunch of flake8 errors that Travis is finding (see above) and I'm also seeing a few warnings when building the docs myself (below). Do you need any advice on tracking down these fixes, or do you want to handle that first? |
|
Sorry about the flake8 errors, I will fix those right away. I'm not sure what to do with the first 8 warnings in your log, since they don't appear to be from I don't see all of the |
isullivan
force-pushed
the
tickets/DM-15684
branch
from
e75520f
to
8cad7ac
Compare
|
Okay I'll see if I can figure those warnings out when I go through it. It's looking like either later today or by Monday at this rate. |
jonathansick
approved these changes
Jan 30, 2019
There was a problem hiding this comment.
Thanks for doing this! I had to time box the review because obviously there's a lot to cover, and a number of things can be refined with further iterative development. I hope that the comments I have provided are at least educational and can frame future work.
In general, an easy win is to ensure that all the descriptions are sentences (capitalized and ending with a period).
I also noticed that there are references to DipoleFitAlgorithm, but that class isn't part of the public API documentation. You might want to look into either making that API public in lsst.ip.diffim or adding another automodapi directive that points to the module where DipoleFitAlgorithm is located (choose whichever is appropriate).
doc/lsst.ip.diffim/index.rst
Outdated
| @@ -12,4 +12,4 @@ Python API reference | |||
| ==================== | |||
|
|
|||
| .. automodapi:: lsst.ip.diffim | |||
| :skip: wrapSimpleAlgorithm | |||
| :skip: wrapSimpleAlgorithm | |||
There was a problem hiding this comment.
At the top of this page, could you change the title from lsst.ip_diffim to lsst.ip.diffim?
| - @ref ip_diffim_diaCatalogSourceSelector_Debug | ||
|
|
||
| @section ip_diffim_diaCatalogSourceSelector_Purpose Description | ||
| """Select sources for Kernel candidates |
There was a problem hiding this comment.
Try to make the class summaries sentences with a full stop, and also switch from imperative (for functions) to a description of the class's purpose, like:
A task that selects sources for Kernel candidates.
| @copydoc \_\_init\_\_ | ||
|
|
||
| @section ip_diffim_diaCatalogSourceSelector_IO Invoking the Task | ||
| Notes |
There was a problem hiding this comment.
Just a heads-up that all this content will eventually move out of the docstring to a task topic page: https://developer.lsst.io/stack/task-topic-type.html
I think it's find to ship this initial numpydoc conversion with this as-is, but keep in mind that this is a TODO item.
| <dt>pauseAtEnd | ||
| <dd>bool; if True wait after displaying everything and wait for user input | ||
| </dl> | ||
| .. code-block :: none |
There was a problem hiding this comment.
You can do .. code-block:: py to get highlighting.
python/lsst/ip/diffim/diffimTools.py
Outdated
|
|
||
| - Uses numpy.random; you may wish to call numpy.random.seed first. | ||
|
|
||
| Returns |
There was a problem hiding this comment.
Move the Returns section above the Notes section. The ordering for the sections is documented at https://developer.lsst.io/python/numpydoc.html#py-docstring-sections
python/lsst/ip/diffim/zogy.py
Outdated
| @@ -712,7 +735,7 @@ def computeScorrFourierSpace(self, xVarAst=0., yVarAst=0., **kwargs): | |||
|
|
|||
| Returns | |||
| ------- | |||
| A lsst.pipe.base.Struct containing: | |||
| A `lsst.pipe.base.Struct` containing: | |||
There was a problem hiding this comment.
I'm seeing this a lot in this module; keep in mind that this isn't the proper format for a Returns section. https://developer.lsst.io/python/numpydoc.html#struct-return-types
python/lsst/ip/diffim/zogy.py
Outdated
| the template exposure, from which a corresponding sub-exposure | ||
| is extracted | ||
| kwargs : | ||
| **kwargs : |
python/lsst/ip/diffim/zogy.py
Outdated
| These kwargs may also include arguments to be propagated to | ||
| `ZogyTask.computeDiffim` and `ZogyTask.computeScorr`. | ||
|
|
||
| - doScorr : `bool` |
There was a problem hiding this comment.
Make this a regular definition list. So leave out the - from the term, and have the content indented.
python/lsst/ip/diffim/zogy.py
Outdated
| additional arguments to be passed to | ||
| `lsst.pipe.base.task.Task.__init__` | ||
| kwargs : | ||
| **kwargs : | ||
| additional keyword arguments to be passed to | ||
| `lsst.pipe.base.task.Task.__init__` |
python/lsst/ip/diffim/zogy.py
Outdated
| @@ -153,10 +153,10 @@ def __init__(self, templateExposure=None, scienceExposure=None, sig1=None, sig2= | |||
| psf2 : 2D `numpy.array` | |||
| (Optional) 2D array containing the PSF image for the science img. If | |||
| `None`, it is extracted from the PSF taken at the center of `scienceExposure`. | |||
| args : | |||
| *args : | |||
| additional arguments to be passed to | |||
| `lsst.pipe.base.task.Task.__init__` | |||
isullivan
force-pushed
the
tickets/DM-15684
branch
from
b5ee7bd
to
96f7b53
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.