DM-11592: Better documentation for ap_verify #12

kfindeisen · 2017-11-29T00:14:00Z

This PR adds new-style documentation to the ap_verify package. Since the package is not a library and has a minimal public-facing API, most of the documentation is instructions for ap_verify users and guides to key ap_verify concepts (in particular, datasets).

There are a number of cross-references to the other ap packages, the verify documentation, the pipe_base documentation, and the butler and obs framework topic documentation. Naturally, these references will need to be updated once these documents actually exist.

Since new-style documentation is not yet built by Travis, I don't have an HTML version of the documentation to link to.

jonathansick

@kfindeisen, looks really good. I've added some suggested changes as line comments.

jonathansick · 2017-12-01T18:03:25Z

doc/ap_verify/index.rst

+ap_verify
+#########
+
+The ``ap_verify`` package wraps :py:mod:`ap_pipe <lsst.ap.pipe>` with support for managing :py:mod:`verify <lsst.verify>` metrics.


For "verify" use the actual module namespace for clarity:

for managing `lsst.verify` metrics.

Note that single back-ticks can be used for any type of Python API reference (being the default role).

So :mod: is also redundant?

jonathansick · 2017-12-01T18:12:32Z

doc/lsst.ap.verify/datasets-install.rst

+-------------
+
+The Dataset framework requires that the computer have version 13.0 or later of the LSST Stack (specifically, the ``obs`` packages and their dependencies) installed.
+`Installing lsst_distrib <https://pipelines.lsst.io/#installation>`_ is the simplest way to ensure all dependencies are satisfied.


:ref:`Installing lsst_distrib <part-installation>` [...]

jonathansick · 2017-12-01T18:14:06Z

doc/lsst.ap.verify/datasets-install.rst

+The Dataset framework requires that the computer have version 13.0 or later of the LSST Stack (specifically, the ``obs`` packages and their dependencies) installed.
+`Installing lsst_distrib <https://pipelines.lsst.io/#installation>`_ is the simplest way to ensure all dependencies are satisfied.
+
+The framework also requires `Git LFS`_ and the `EUPS`_ package management system. The latter is usually included by the Stack installation process.


Second sentence:

EUPS is included in the Stack installation.

jonathansick · 2017-12-01T18:15:52Z

doc/lsst.ap.verify/datasets-install.rst

+
+.. TODO: should we have a proper versioning system for datasets? (DM-12853)
+
+Once the dataset has been installed, use ``eups declare`` to register the downloaded directory.


Try to wrap inline shell commands in the command role, like

:command:`eups declare`

jonathansick · 2017-12-01T18:17:23Z

doc/lsst.ap.verify/datasets-install.rst

+
+.. code-block:: sh
+
+   $ git clone https://github.com/lsst/ap_verify_hits2015 mydata


I've been leaving out any $ or > characters to reduce copy-paste errors. There are special directives for making uncopyable prompts but I've had some trouble with them.

jonathansick · 2017-12-01T18:44:29Z

doc/lsst.ap.verify/running.rst

+
+``ap_verify`` is a Python script designed to be run on both developer machines and verification servers.
+This page describes the minimum options needed to run ``ap_verify``.
+For more details, see the :ref:`ap-verify-cmd` or run ``ap_verify`` with the ``-h`` argument.


[...] or run :option:`ap_verify.py -h`.

(links to the reference docs).

jonathansick · 2017-12-01T18:45:47Z

python/lsst/ap/verify/ap_verify.py

    """Measure any metrics that apply to the final result of the AP pipeline,
    rather than to a particular processing stage.

    Parameters
    ----------
    metadata: `lsst.daf.base.PropertySet`
        The metadata produced by the AP pipeline.
-    metrics_job: `verify.Job`
+    metricsJob: `verify.Job`


lsst.verify.Job

— needs the actual namespace to make the link.

Huh. It looks like the dev guide contradicts itself here:

https://developer.lsst.io/docs/py_docs.html#parameter-types says to use the full namespace

https://developer.lsst.io/docs/py_docs.html#see-also says to start with the lowest common ancestor

Does the link generation actually work differently in these two contexts?

Ah, I may need to update those docs. I guess we'll see if the link works when it's integrated into pipelines.lsst.io.

Related question: should exceptions also be fully qualified? There are a few that are not in __builtins__, and the dev guide doesn't say.

Yes, fully-qualified.

jonathansick · 2017-12-01T18:47:26Z

python/lsst/ap/verify/measurements/compute_metrics.py

-def measure_from_metadata(metadata):
-    """Attempts to compute all known metrics on Task metadata.
+def measureFromMetadata(metadata):
+    """Attempt to compute all known metrics on Task metadata.


Leave out the "Attempt to" part.

jonathansick · 2017-12-01T18:49:30Z

python/lsst/ap/verify/measurements/compute_metrics.py


    Raises
    ------
-    `RuntimeError`:
-        the config file exists, but does not contain the expected data
+    RuntimeError:


No colon after the exception in the Raises section

Raises ------ RuntimeError The ``ap_verify`` configuration file exists, but does not contain the expected data under ``measurements``.

(check the Raises sections of other docstrings)

jonathansick · 2017-12-01T18:52:07Z

python/lsst/ap/verify/measurements/profiling.py

        The fully qualified name of the metric being measured, e.g.,
        "pipe_tasks.ProcessCcdTime"

    Returns
    -------
-    an `lsst.verify.Measurement` for `metric_name`, or `None` if the timing
-    information for `task_name` is not present in `metadata`
+    an `lsst.verify.Measurement` for `metricName`, or `None` if the timing


Returns is formatted like Parameters, even if you need to make up the name of the return variable. For example, I'd write this one as:

Returns ------- measurement : `lsst.verify.Measurement` The metric measurement, or `None` if timing information for ``taskName`` is not present in the ``metadata``.

ebellm

Looks good overall--just need to make a clarification about parallelization.

ebellm · 2017-12-14T21:47:59Z

doc/lsst.ap.verify/running.rst

+
+.. prompt:: bash
+
+   python ap_verify/bin/ap_verify.py --dataset HiTS2015 --output workspace/hits/ --dataIdString "visit=54123 ccd=25 filter=g" --silent


I would note here that running ap_verify in parallel causes the metric output to be clobbered, and reference DM-13042 (which is the ticket to fix that).

ebellm · 2017-12-14T21:48:20Z

doc/lsst.ap.verify/running.rst

+How to Use Measurements of Metrics
+----------------------------------
+
+After ``ap_verify`` has run, it will produce a file named :file:`ap_verify.verify.json` in the working directory.


I would make the same comment here about parallelization also.

kfindeisen · 2017-12-14T22:19:36Z

A compiled version [zip] of the latest docs.

ebellm · 2017-12-14T22:23:48Z

doc/lsst.ap.verify/running.rst

+
+.. warning::
+
+   ``ap_verify.py`` is not designed to have multiple instances running concurrently.


how about "...does not currently support running multiple instances concurrently." instead.

kfindeisen requested review from jonathansick and ebellm November 29, 2017 00:14

jonathansick reviewed Dec 1, 2017

View reviewed changes

kfindeisen added 4 commits December 5, 2017 17:29

Conform to LSST naming style.

baa0390

Skeleton for Sphinx package documentation.

01f0355

Write ap_verify package doc.

2684204

Add dataset documentation.

c24be65

kfindeisen force-pushed the tickets/DM-11592 branch 2 times, most recently from 4f8ce9a to 32fd6aa Compare December 6, 2017 23:20

ebellm requested changes Dec 14, 2017

View reviewed changes

kfindeisen force-pushed the tickets/DM-11592 branch from 32fd6aa to eb71f9a Compare December 14, 2017 22:12

ebellm reviewed Dec 14, 2017

View reviewed changes

kfindeisen added 8 commits December 14, 2017 14:30

Transfer how-to guide.

7c8efb6

Add command-line reference.

b20d5e5

Add config documentation.

85e5a99

Discuss error-handling policy.

d93739c

Add overview.

48e53a7

Add ap.verify.measurements documentation.

c9fd80c

Clean up API documentation.

6ea59d6

Fix API visibility bug.

9040f71

kfindeisen force-pushed the tickets/DM-11592 branch from eb71f9a to 9040f71 Compare December 14, 2017 22:30

ebellm approved these changes Dec 14, 2017

View reviewed changes

kfindeisen merged commit 9040f71 into master Dec 14, 2017

kfindeisen deleted the tickets/DM-11592 branch November 30, 2018 22:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-11592: Better documentation for ap_verify #12

DM-11592: Better documentation for ap_verify #12

kfindeisen commented Nov 29, 2017

jonathansick left a comment

jonathansick Dec 1, 2017

kfindeisen Dec 4, 2017

jonathansick Dec 1, 2017

jonathansick Dec 1, 2017

jonathansick Dec 1, 2017

jonathansick Dec 1, 2017

jonathansick Dec 1, 2017

jonathansick Dec 1, 2017

kfindeisen Dec 5, 2017

jonathansick Dec 5, 2017

kfindeisen Dec 6, 2017 •

edited

jonathansick Dec 6, 2017

jonathansick Dec 1, 2017

jonathansick Dec 1, 2017

jonathansick Dec 1, 2017

ebellm left a comment

ebellm Dec 14, 2017

ebellm Dec 14, 2017

kfindeisen commented Dec 14, 2017

ebellm Dec 14, 2017

kfindeisen Dec 14, 2017


		.. TODO: should we have a proper versioning system for datasets? (DM-12853)

		Once the dataset has been installed, use ``eups declare`` to register the downloaded directory.


		.. code-block:: sh

		$ git clone https://github.com/lsst/ap_verify_hits2015 mydata


		.. prompt:: bash

		python ap_verify/bin/ap_verify.py --dataset HiTS2015 --output workspace/hits/ --dataIdString "visit=54123 ccd=25 filter=g" --silent


		.. warning::

		``ap_verify.py`` is not designed to have multiple instances running concurrently.

DM-11592: Better documentation for ap_verify #12

DM-11592: Better documentation for ap_verify #12

Conversation

kfindeisen commented Nov 29, 2017

jonathansick 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

kfindeisen Dec 6, 2017 • edited

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

ebellm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfindeisen commented Dec 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfindeisen Dec 6, 2017 •

edited