DM-15681: convert skymap documentation to numpydoc #25

DavidStaker · 2018-09-07T23:05:30Z

No description provided.

parejkoj

I've left some comments: see if you can apply them to the rest of the code, and I'll take another look (so I'm not making the same comment repeatedly). Please ask questions if my comments aren't clear (or if I got something wrong about the documentation policy).

doc/lsst.skymap/index.rst

doc/skymap/index.rst

python/lsst/skymap/baseSkyMap.py

python/lsst/skymap/detail/dodecahedron.py

parejkoj · 2018-09-12T21:07:39Z

python/lsst/skymap/detail/dodecahedron.py

-        @return a list of face centers (in index order); each a unit vector (numpy array)
+        Returns
+        -------
+        results : `self.faceVecList[:]`


that's not a type

Does this also just count as "callable"?

I think it's a np.ndarray.

python/lsst/skymap/detail/dodecahedron.py

jonathansick · 2018-10-11T18:44:11Z

Hey @DavidStaker, just to alert you that something might be wonky with the Git history here. Generally a PR shouldn't have a merge commit from another PR (did you update your branch using the green button)? There's also some Git merge conflict markup in doc/lsst.skymap/index.rst.

Made my own edits: Russell is reviewing now.

r-owen

Please take another pass. If you want help I am willing.

r-owen · 2018-12-18T00:58:38Z

doc/lsst.skymap/index.rst

@@ -0,0 +1,47 @@
+.. py:currentmodule:: lsst.skymap
+
+.. _lsst.skymap:


I think this is redundant with the title on line 6 and on my home Mac that redundancy gives a warning

This is how it is shown in the stack template example:

https://raw.githubusercontent.com/lsst/templates/master/project_templates/stack_package/example/doc/lsst.example/index.rst

There shouldn't be a pre-existing label with this name. I'll take a look around the package to see what's up.

I can't reproduce the warning with the lsst.skymap label.

These are the only warnings I get:

/Users/jsick/lsst/pipelines_lsst_io_stack/stacks/d_2018_12_07/stack/miniconda3-4.5.4-fcd27eb/DarwinX86/pex_config/16.0-8-g4734f7a/python/lsst/pex/config/config.py:docstring of lsst.skymap.BaseSkyMapConfig.compare:12: WARNING: Unexpected indentation. /Users/jsick/lsst/pipelines_lsst_io_stack/stacks/d_2018_12_07/stack/miniconda3-4.5.4-fcd27eb/DarwinX86/pex_config/16.0-8-g4734f7a/python/lsst/pex/config/config.py:docstring of lsst.skymap.BaseSkyMapConfig.loadFromStream:7: WARNING: Unexpected indentation. /Users/jsick/lsst/pipelines_lsst_io_stack/stacks/d_2018_12_07/stack/miniconda3-4.5.4-fcd27eb/DarwinX86/pex_config/16.0-8-g4734f7a/python/lsst/pex/config/config.py:docstring of lsst.skymap.DodecaSkyMapConfig.compare:12: WARNING: Unexpected indentation. /Users/jsick/lsst/pipelines_lsst_io_stack/stacks/d_2018_12_07/stack/miniconda3-4.5.4-fcd27eb/DarwinX86/pex_config/16.0-8-g4734f7a/python/lsst/pex/config/config.py:docstring of lsst.skymap.DodecaSkyMapConfig.loadFromStream:7: WARNING: Unexpected indentation. /Users/jsick/lsst/pipelines_lsst_io_stack/stacks/d_2018_12_07/stack/miniconda3-4.5.4-fcd27eb/DarwinX86/pex_config/16.0-8-g4734f7a/python/lsst/pex/config/config.py:docstring of lsst.skymap.EquatSkyMapConfig.compare:12: WARNING: Unexpected indentation. /Users/jsick/lsst/pipelines_lsst_io_stack/stacks/d_2018_12_07/stack/miniconda3-4.5.4-fcd27eb/DarwinX86/pex_config/16.0-8-g4734f7a/python/lsst/pex/config/config.py:docstring of lsst.skymap.EquatSkyMapConfig.loadFromStream:7: WARNING: Unexpected indentation. /Users/jsick/lsst/pipelines_lsst_io_stack/stacks/d_2018_12_07/stack/miniconda3-4.5.4-fcd27eb/DarwinX86/pex_config/16.0-8-g4734f7a/python/lsst/pex/config/config.py:docstring of lsst.skymap.baseSkyMap.BaseSkyMapConfig.compare:12: WARNING: Unexpected indentation. /Users/jsick/lsst/pipelines_lsst_io_stack/stacks/d_2018_12_07/stack/miniconda3-4.5.4-fcd27eb/DarwinX86/pex_config/16.0-8-g4734f7a/python/lsst/pex/config/config.py:docstring of lsst.skymap.baseSkyMap.BaseSkyMapConfig.loadFromStream:7: WARNING: Unexpected indentation. /Users/jsick/lsst/pipelines_lsst_io_stack/stacks/d_2018_12_07/stack/miniconda3-4.5.4-fcd27eb/DarwinX86/pex_config/16.0-8-g4734f7a/python/lsst/pex/config/config.py:docstring of lsst.skymap.healpixSkyMap.HealpixSkyMapConfig.compare:12: WARNING: Unexpected indentation. /Users/jsick/lsst/pipelines_lsst_io_stack/stacks/d_2018_12_07/stack/miniconda3-4.5.4-fcd27eb/DarwinX86/pex_config/16.0-8-g4734f7a/python/lsst/pex/config/config.py:docstring of lsst.skymap.healpixSkyMap.HealpixSkyMapConfig.loadFromStream:7: WARNING: Unexpected indentation. /Users/jsick/lsst/pipelines_lsst_io_stack/stacks/d_2018_12_07/stack/miniconda3-4.5.4-fcd27eb/DarwinX86/pex_config/16.0-8-g4734f7a/python/lsst/pex/config/config.py:docstring of lsst.skymap.ringsSkyMap.RingsSkyMapConfig.compare:12: WARNING: Unexpected indentation. /Users/jsick/lsst/pipelines_lsst_io_stack/stacks/d_2018_12_07/stack/miniconda3-4.5.4-fcd27eb/DarwinX86/pex_config/16.0-8-g4734f7a/python/lsst/pex/config/config.py:docstring of lsst.skymap.ringsSkyMap.RingsSkyMapConfig.loadFromStream:7: WARNING: Unexpected indentation. looking for now-outdated files... none found pickling environment... done checking consistency... done preparing documents... done writing output... [100%] py-api/lsst.skymap.ringsSkyMap.RingsSkyMapConfig generating indices... writing additional pages... search copying static files... done copying extra files... done dumping search index in English (code: en) ... done dumping object inventory... done build succeeded, 12 warnings. The HTML pages are in doc/_build/html.

@r-owen what version of Sphinx do you have installed?

Oh, that's dope. I'm still very interested in getting to the bottom of the warning Russell is experiencing.

I was seeing it at home. I'll have to check it there.

I forgot to do this last night. But I am confused. @jonathansick why isn't the link redundant? I thought all titles made links automatically, so:

########### lsst.skymap ###########

should make an anchor lsst.skymap which is the same as the anchor above that my build complained about.

If you use a link target, like

.. _lsst.skymap: ########### lsst.skymap ###########

It means you can link to that page/title with a ref:

:ref:`lsst.skymap`

Without the label, the ref role won't make a link. Now, if it does, that means that another lsst.skymap label is defined elsewhere, which wouldn't be good.

It's true that Sphinx always autoslugifies headings and turns those into permalink, regardless of their being a link target associated with the header. But those automatic anchor links are only accessible from HTML, and aren't accessible from reStructuredText's ref role.

@jonathansick thank you for the explanation. Today when I got home I tried package-docs clean followed by package-docs build and saw no warnings. I don't know where that warning came from.

r-owen · 2018-12-18T00:59:49Z

doc/lsst.skymap/index.rst

+``lsst.skymap`` is developed at https://github.com/lsst/skymap.
+You can find Jira issues for this module under the `skymap <https://jira.lsstcorp.org/issues/?jql=project%20%3D%20DM%20AND%20component%20%3D%20skymap>`_ component.
+
+.. If there are topics related to developing this module (rather than using it), link to this from a toctree placed here.


I suggest deleting this line

r-owen · 2018-12-18T01:00:36Z

doc/lsst.skymap/index.rst

+.. If there are topics related to developing this module (rather than using it), link to this from a toctree placed here.
+
+.. .. toctree::
+..    :maxdepth: 1


I think this is redundant with the toctree on line 19 (but examine the built html if in doubt).

Again, this is in the template example.

It isn't redundant.
https://developer.lsst.io/v/u-jonathansick-package-doc-clarification/stack/module-homepage-topic-type.html#using-module-and-toctrees versus
https://developer.lsst.io/v/u-jonathansick-package-doc-clarification/stack/module-homepage-topic-type.html#contributing-section

But you can delete the comment right now if you like.

r-owen · 2018-12-18T01:03:38Z

doc/lsst.skymap/index.rst

+   :no-main-docstr:
+   :no-inheritance-diagram:
+.. automodapi:: lsst.skymap.detail
+.. automodapi:: lsst.skymap.baseSkyMap


Is this needed? baseSkyMap is included by skymap/__init__.py unlike the others (detail, cachingSkyMap...). I'm a bit surprised that the modules listed in lines 45-47 are not included, but they aren't and changing that may be out of scope (though I wouldn't complain if you did it).

r-owen · 2018-12-18T01:09:58Z

python/lsst/skymap/baseSkyMap.py

+        Parameters
+        ----------
+        config : `BaseSkyMapConfig` or None
+            The configuration for this SkyMap; if None use the default config.


The parameters should be moved to the class doc string any __init__.py should not have any doc string. https://developer.lsst.io/python/numpydoc.html#documenting-classes

Sorry about that. I wrote a lot of this before that rule and until that rule I always documented __init__.py.

Please look for this everywhere in the package.

r-owen · 2018-12-18T01:59:53Z

python/lsst/skymap/ringsSkyMap.py

+
+        Returns
+        -------
+        tractList :


Specify the type as `list` of `TractInfo` and simplify the text on the next line (e.g. "Tracts which include...").

r-owen · 2018-12-18T02:00:35Z

python/lsst/skymap/ringsSkyMap.py


-        @note
+        Notes
+        -----


Again the note is obsolete (sorry I missed that when I got rid of afw.Coord).

r-owen · 2018-12-18T02:01:02Z

python/lsst/skymap/ringsSkyMap.py

+        Parameters
+        ----------
+        coordList : `list` of `lsst.geom.SpherePoint`
+            list of sky coordinates


ICRS sky coordinates

r-owen · 2018-12-18T02:04:06Z

python/lsst/skymap/tractInfo.py

@@ -364,7 +426,7 @@ def __init__(self, ident, patchInnerDimensions, patchBorder, ctrCoord, radius, t
        self._vertexCoordList = finalWcs.pixelToSky(bboxD.getCorners())

    def _minimumBoundingBox(self, wcs):
-        """The minimum bounding box is calculated using the nominated radius"""
+        """The minimum bounding box is calculated using the nominated radius."""


Consider rewording as a proper doc string, e.g.

"""Calculate the minimum bounding box for the tract, given the WCS, and the nominated radius. """

r-owen · 2018-12-18T02:05:54Z

python/lsst/skymap/tractInfo.py

-        @warning: this is not a deep copy
+        Notes
+        -----
+        **Warning:** this is not a deep copy


SkyWcs is immutable, so this note is obsolete.

I suggest specifying the return type of afw.math.SkyWcs.

parejkoj · 2018-12-18T23:00:53Z

Ok, I think I took care of all your comments, @r-owen. Still no build warnings for me.

r-owen

Some of the things I looked at haven't been fixed. I didn't check the others.

r-owen · 2018-12-18T23:25:41Z

doc/lsst.skymap/index.rst

@@ -0,0 +1,47 @@
+.. py:currentmodule:: lsst.skymap
+
+.. _lsst.skymap:


I was seeing it at home. I'll have to check it there.

r-owen · 2018-12-18T23:28:32Z

python/lsst/skymap/detail/dodecahedron.py


 class Dodecahedron:
    """A dodecahedron

-    Contains positions of faces and associated vertices


Still no docs for the constructor parameter here or for DodecaSkyMap. Please look harder for this. All classes should document their constructor arguments.

r-owen · 2018-12-18T23:32:26Z

python/lsst/skymap/baseSkyMap.py

        - This routine will be more efficient if coord is ICRS.
-        - If coord is equidistant between multiple sky tract centers then one is arbitrarily chosen.
-        - The default implementation is not very efficient; subclasses may wish to override.
+


I still find 4 instances of this. Did you push your changes?

parejkoj · 2018-12-18T23:36:24Z

I think I hadn't pushed between going to your office and you looking at it. It is pushed now (your above comments no longer apply).

r-owen

Still some changes to make, but at this point I'd say make them and merge.

r-owen · 2018-12-19T19:11:05Z

python/lsst/skymap/baseSkyMap.py

+        -------
+        reList : `list` of `TractInfo` or `PatchInfo`
+            For tracts and patches that contain, or may contain, the specified
+            region. The list will be empty if there is no overlap.


The return type is wrong. This function returns `list` of (`TractInfo`, `list` of `PatchInfo`)

See RingSkyMap.py for a skymap that has this correct. Please make them the same. (You could delete the version in RingSkyMap if the rendered document picks it up from the base class).

r-owen · 2018-12-19T19:12:53Z

python/lsst/skymap/baseSkyMap.py

+        Returns
+        -------
+        result : `distTractInfoList`
+            TractInfo of tract whose center is nearest the specified coord.


This is wrong. It should read tractInfo : `TractInfo`

r-owen · 2018-12-19T19:19:26Z

python/lsst/skymap/baseSkyMap.py

        all subclasses, but called only by the base class implementation of
-        `getSha1()`.
+        `getSha1` .


Also for the register method below: the name and registry arguments should be documented. @TallJimbo wrote this code so I am hoping he can weigh in on this.

name is of type str and if Jim cannot provide a better description then I suggest you use "Name of sky map" but it is too vague to make me happy.
I don't know the data type for a Gen3 butler registry. Jim?

r-owen · 2018-12-19T19:22:16Z

doc/lsst.skymap/index.rst

@@ -0,0 +1,47 @@
+.. py:currentmodule:: lsst.skymap
+
+.. _lsst.skymap:


I forgot to do this last night. But I am confused. @jonathansick why isn't the link redundant? I thought all titles made links automatically, so:

########### lsst.skymap ###########

should make an anchor lsst.skymap which is the same as the anchor above that my build complained about.

r-owen · 2018-12-19T19:25:23Z

python/lsst/skymap/baseSkyMap.py

@@ -76,24 +76,30 @@ class BaseSkyMap:

    See TractInfo for more information.

+    Parameters
+    ----------
+    config : `BaseSkyMapConfig` or None


I suggest config : `BaseSkyMapConfig` (optional)
and look for all other instances of config : because there is a wide variation in how this is listed.

I cleaned it up as best I could: there's also variation in the __init__ call signatures (some default None, some don't).

r-owen · 2018-12-19T20:05:34Z

python/lsst/skymap/tractInfo.py

+        Returns
+        -------
+        finalBBox : `lsst.geom.Box2I`
+            revised bounding box,


"Revised bounding box" (with or without a final period). Please don't end with a comma.

r-owen · 2018-12-19T20:05:50Z

python/lsst/skymap/tractInfo.py

+        finalBBox : `lsst.geom.Box2I`
+            revised bounding box,
+        wcs : `lsst.afw.geom.SkyWcs`
+            revised Wcs


"Revised WCS"

r-owen · 2018-12-19T20:07:12Z

python/lsst/skymap/tractInfo.py

+        ICRS sky coordinate of center of inner region of tract; also used as
+        the CRVAL for the WCS.
+    vertexCoordList : `list` of `lsst.afw.geom.SpherePoint`
+        Of vertices that define the boundaries of the inner region.


"Vertices that define" or "List of vertices that define". Looks like an error from automatic conversion.

r-owen · 2018-12-19T20:12:10Z

python/lsst/skymap/tractInfo.py

+        Notes
+        -----
+        warning:
+        - This may give incorrect answers on regions that are larger than a tract


Please fix to make warning bold and render the items as an unordered list:

**warning:** - This may give... - This uses... (especially if..

r-owen · 2018-12-19T20:13:37Z

python/lsst/skymap/tractInfo.py

+        Returns
+        -------
+        result : `list` of `lsst.skymap.PatchInfo`
+            List of PatchInfo for patches that contain, or may contain, the specified region.


This line is too long. The warnings below are also too long.

Update doc/ directory to contain pipelines RST files. Fix `__all__`

DavidStaker requested a review from parejkoj September 7, 2018 23:05

DavidStaker changed the title ~~all to numpydoc - WIP~~ tickets/DM-15681 - convert skymap documentation to numpydoc Sep 10, 2018

parejkoj previously requested changes Sep 12, 2018

View reviewed changes

parejkoj force-pushed the tickets/DM-15681 branch from d501ffa to 4060c64 Compare December 18, 2018 00:39

r-owen requested changes Dec 18, 2018

View reviewed changes

r-owen approved these changes Dec 19, 2018

View reviewed changes

WERKSQUAD and others added 2 commits December 19, 2018 13:06

Convert docstrings to numpydoc

5a8e7c4

Update doc/ directory to contain pipelines RST files. Fix `__all__`

Update to use lsst.geom in test

10b8c01

parejkoj force-pushed the tickets/DM-15681 branch from 2ca9271 to 10b8c01 Compare December 19, 2018 21:06

parejkoj changed the title ~~tickets/DM-15681 - convert skymap documentation to numpydoc~~ DM-15681: convert skymap documentation to numpydoc Dec 19, 2018

parejkoj merged commit 5858431 into master Dec 19, 2018

		@@ -0,0 +1,47 @@
		.. py:currentmodule:: lsst.skymap

		.. _lsst.skymap:

DM-15681: convert skymap documentation to numpydoc #25

DM-15681: convert skymap documentation to numpydoc #25

Conversation

DavidStaker commented Sep 7, 2018

parejkoj 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

jonathansick commented Oct 11, 2018

r-owen 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

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

parejkoj commented Dec 18, 2018

r-owen 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

parejkoj commented Dec 18, 2018

r-owen 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