DM-15554: Convert pipe_tasks to numpydoc status #219

DavidStaker · 2018-09-07T23:04:26Z

No description provided.

mrawls

I have gone through a couple of the modules and found consistent things that need to be changed throughout. Can you please make those changes and then I'll review the rest? Let me know if anything I said isn't clear!

doc/lsst.pipe.tasks/index.rst

mrawls · 2018-09-11T18:48:39Z

python/lsst/pipe/tasks/calibrate.py


+
+    Examples


A lot of the "examples" here (and throughout pipe_tasks) don't really stand alone. I think an "Examples" doc section should show an example of using the class or object just documented in a general sense. In contrast, this is just an example of how to use a custom debugger. I suggest absorbing this and similar mini-examples into the "Notes" section.

Hmmm, understood. I'm not sure if this applied to many .py files in pipe_tasks or not, but a lot of the "Examples" sections come from when someone wrote a lot of doxygen documentation - but the example dox doc pages would often link to code that wasn't there anymore which resulted in a lot of empty white boxes. So I would cut out almost everything but still left the remaining part of the section. I'll go through and fix these.

mrawls · 2018-09-11T18:51:15Z

python/lsst/pipe/tasks/calibrate.py

            Schema values specified in config.icSourceFieldsToCopy will be
            taken from this schema. If set to None, no values will be
            propagated from the icSourceCatalog
-        @param[in,out] kwargs  other keyword arguments for
+        kwargs :


The format for documenting kwargs is here.

mrawls · 2018-09-11T18:52:56Z

python/lsst/pipe/tasks/calibrate.py

@@ -278,26 +266,33 @@ def DebugInfo(name):

    def __init__(self, butler=None, astromRefObjLoader=None,
                 photoRefObjLoader=None, icSourceSchema=None, **kwargs):


init methods should not have docstrings, and instead this info should be moved up to the class level. See here.

mrawls · 2018-09-11T18:57:26Z

python/lsst/pipe/tasks/calibrate.py

-        @param[in] butler  The butler is passed to the refObjLoader constructor
+        Parameters
+        ----------
+        butler :


This (and many other parameters) is missing a type after the name. This one should read butler : lsst.daf.persistence.Butler, for example. You can find a lot of the common ones in the assembleCoadd.py module that I numpydoc-ified a few months ago. This is tedious but really important so users can understand what kind of object each parameter is.

mrawls · 2018-09-11T19:10:42Z

python/lsst/pipe/tasks/characterizeImage.py

-        - exposure: characterized exposure; image is repaired by interpolating over cosmic rays,
+        Returns
+        -------
+        dmeRes : `struct`


This is super close to the right format for a struct, but needs some tweaking (see earlier comment).

mrawls · 2018-09-11T19:11:51Z

python/lsst/pipe/tasks/characterizeImage.py

-        @param[in] butler  A butler object is passed to the refObjLoader constructor in case
+        Parameters
+        ----------
+        butler :


Again, this and all subsequent parameters need a type

mrawls · 2018-09-11T19:13:29Z

python/lsst/pipe/tasks/dcrAssembleCoadd.py

@@ -264,6 +264,8 @@ def run(self, skyInfo, tempExpRefList, imageScalerList, weightList,
            supplementaryData=None):
        """Assemble the coadd.

+        Notes


I don't think this needs to be in a Notes section, it can just be the extended description as-is.

@mrawls No, The extended description should be at most a few sentences. We can't allow the extended description to push the parameter and returns descriptions too far down the page. Notes is totally appropriate and necessary here.

https://developer.lsst.io/python/numpydoc.html#extended-summary

My bad, I didn't catch that the "Notes" was more than the next sentence that followed! Completely agree with @jonathansick, this is fine as-is.

mrawls · 2018-09-11T19:13:41Z

python/lsst/pipe/tasks/dcrAssembleCoadd.py

@@ -283,7 +285,7 @@ def run(self, skyInfo, tempExpRefList, imageScalerList, weightList,

        Once the ``DcrModel`` reaches convergence or the maximum number of
        iterations has been reached, fill the metadata for each subfilter
-        image and make them proper ``coaddExposure``s.
+        image and make them proper ``coaddExposure`` .


Not clear why you removed the plural 's' here?

Anything with the format words causes the numpydoc build to complain.

The solution is:

``coaddExposure``\ 's.

https://developer.lsst.io/restructuredtext/style.html#inline-text-styling

...and then you need to tell python it's a r""" string to prevent warnings about unknown escapes.

mrawls · 2018-09-11T19:15:33Z

python/lsst/pipe/tasks/dcrAssembleCoadd.py

@@ -297,7 +299,6 @@ def run(self, skyInfo, tempExpRefList, imageScalerList, weightList,
            The weight to give each input exposure in the coadd
        supplementaryData : `lsst.pipe.base.Struct`
            Result struct returned by ``makeSupplementaryData`` with components:
-


I think this white space is correct, see here

jonathansick · 2018-10-11T18:48:12Z

Hey @DavidStaker, I just want to alert you that the Git history here is a little wonky. I think (forgive me if I'm wrong) that this branch might have been updated with the green button on the PR page. This has the effect or merging master into your PR's branch, which isn't what we do. We do a git rebase -based workflow described here:
https://developer.lsst.io/work/flow.html#pushing-code

I'm sure someone at UW can walk you through the rebase if you need assistance.

Lots of work to do to finish this; original review is outdated.

See ticket comment on details. Minor style corrections.

parejkoj · 2022-09-15T22:24:06Z

Just a note to say that we should not delete this branch yet, as we are still using it as reference for the ongoing doxygen->numpydoc conversion (see DM-35939).

DavidStaker requested a review from mrawls September 7, 2018 23:04

DavidStaker changed the title ~~Tickets/dm 15554 - WIP~~ Tickets/dm 15554 - Convert pipe_tasks to numpydoc status Sep 10, 2018

mrawls previously requested changes Sep 11, 2018

View reviewed changes

mrawls changed the title ~~Tickets/dm 15554 - Convert pipe_tasks to numpydoc status~~ DM-15554: Convert pipe_tasks to numpydoc status Sep 11, 2018

kgabor force-pushed the tickets/DM-15554 branch 3 times, most recently from e6a5e2a to 87b7a13 Compare December 18, 2018 07:03

mrawls force-pushed the tickets/DM-15554 branch 2 times, most recently from 3399b68 to 14ec627 Compare December 19, 2018 22:39

WERKSQUAD and others added 2 commits December 19, 2018 14:47

Initial work on numpydoc conversion

9c23429

Manual documentation and __all__ updates following rebase to master.

07a0009

See ticket comment on details. Minor style corrections.

mrawls force-pushed the tickets/DM-15554 branch from 14ec627 to 07a0009 Compare December 19, 2018 22:47

mrawls added 3 commits December 19, 2018 16:55

Standardize task doc boilerplate

cb14665

Move assembleCoadd task docs to new home

f544e3f

Clean up calibrate numpydoc

070a161

mrawls force-pushed the tickets/DM-15554 branch from 97856c3 to 070a161 Compare December 20, 2018 01:08

Numpydoc cleanup for CharacterizeImage, MeasurePsf, and Repair.

2bbbe96

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-15554: Convert pipe_tasks to numpydoc status #219

DM-15554: Convert pipe_tasks to numpydoc status #219

DavidStaker commented Sep 7, 2018

mrawls left a comment

mrawls Sep 11, 2018

DavidStaker Sep 18, 2018

mrawls Sep 11, 2018

mrawls Sep 11, 2018

mrawls Sep 11, 2018

mrawls Sep 11, 2018

mrawls Sep 11, 2018

mrawls Sep 11, 2018

jonathansick Sep 18, 2018

mrawls Sep 18, 2018

mrawls Sep 11, 2018

DavidStaker Sep 18, 2018

jonathansick Sep 18, 2018

timj Sep 18, 2018

mrawls Sep 11, 2018

jonathansick commented Oct 11, 2018

parejkoj commented Sep 15, 2022

		@@ -278,26 +266,33 @@ def DebugInfo(name):

		def __init__(self, butler=None, astromRefObjLoader=None,
		photoRefObjLoader=None, icSourceSchema=None, **kwargs):

DM-15554: Convert pipe_tasks to numpydoc status #219

Are you sure you want to change the base?

DM-15554: Convert pipe_tasks to numpydoc status #219

Conversation

DavidStaker commented Sep 7, 2018

mrawls left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathansick commented Oct 11, 2018

parejkoj commented Sep 15, 2022