Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

DM-15056: Fix lsst.afw.geom documentation build issue and misc doc improvements #371

Merged
merged 11 commits into from Jul 13, 2018

Conversation

jonathansick
Copy link
Member

@jonathansick jonathansick commented Jul 12, 2018

The primary purpose of this PR is to fix a Sphinx build failure with the lsst.afw.geom API reference docs. I think the issue is that since many of the APIs are primarily part of lsst.geom, some layer of the auto doc system was filtering those APIs out, while other parts kept those APIs. The interim solution was to ultimately skip those APIs using automodapi's skip argument.

This PR also includes some minor doc improvements to regularize the doc system and fix warnings:

  • Relocated and improved table formatting of the FITS header reference topic
  • Improved docstrings that were creating warnings
  • Relocated the "Footprint" topic
  • Improved titles of the SpanSet and Footprint topics

stack-os-matrix: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/28199/pipeline

- Fix the literals formatting in computePixelToDistortedPixel.
- Add cross references and other minor syntax improvements.
These objects were generating doctree not found errors (See DM-15056). I
think this is because some code was filtering out lsst.geom APIs, but
not fully. Manually filtering them in the automodapi directive solves
the problem for now.
This is what we want, design-wise.
This adds the FITS header reference page into the right directory. This
also sets up the standardized structure of the lsst.afw.image module
homepage.
@jonathansick
Copy link
Member Author

The lsst.afw.geom page:

screenshot_2018-07-12 lsst afw geom afw

@jonathansick jonathansick requested a review from r-owen July 12, 2018 20:52
Copy link
Contributor

@r-owen r-owen left a comment

Choose a reason for hiding this comment

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

Some very nice changes here. I did have a few optional in-line requests (especially updating the FITS keywords for xy0).

My other substantial request is to please show all the "how to" documentation in the top-level afw page, rather than putting it in the sub-package documentation. It would be a lot easier to find, and a user might not know which subpackage they want and thus might never even know that a useful "how to" is available for the topic they are interested in.

#######
#########################################################
Representing regions of pixels with lsst.afw.geom.SpanSet
#########################################################

Copy link
Contributor

Choose a reason for hiding this comment

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

Consider "Using lsst.afw.geom.SpanSet to represent irregular regions of pixels"

because it better emphasizes the class name (similar to your change to the title for Footprint) and because "irregular" is a useful reminder that a SpanSet is probably not rectangular.

:header-rows: 1

* - Keyword
- Format
- Units
- LSST object and accessor [1]_
- Description
- Accessor [1]_ and description

* - ``LTV1``
Copy link
Contributor

Choose a reason for hiding this comment

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

LTV1 and LTV2 no longer hold XY0 (those keywords are still written, though we probably should not mention that, as LSST code never reads them). XY0 is now encoded as WCS "A" using the following keywords:

  • CRPIX[12]A = 1
  • CRVAL[12]A = xy0
  • CTYPE[12]A = "LINEAR"
  • CUNIT[12]A = "PIXEL"
    where [12] means 1 or 2, so CRPIX1 = CRPIX2 = 1, CRVAL1A = xy0[0], CRVAL2A = xy0[1], etc.

If you are willing, please update the documentation accordingly.

Copy link
Member Author

Choose a reason for hiding this comment

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

screen shot 2018-07-13 at 11 02 37 am

@r-owen Here's my update. Does this presentation make sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

(I'll add CUNIT2A)

Copy link
Contributor

@r-owen r-owen Jul 13, 2018

Choose a reason for hiding this comment

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

Two minor corrections: CRPIX and CRVAL are floating point (double if you want to be precise), not integer, because this is the FITS standard for those keywords. CRVAL is rounded to the nearest integer to get XY0, though I don't know if that needs to be said. It is always written as an integer that has been cast to double, so it should always be obvious what the intended value is. Still, a user staring at N.9999999... in a FITS header may wonder if that will be truncated or rounded, so perhaps it's worth saying.

Other than that it looks great. Thank you very much for updating that information!

This will be easier to maintain than the drawn tables.
This makes the table a bit easier to read since it allows the
description column to be wider. The accessor is still quite apparent
thanks to its formatting, and role as a link to the API reference.
@jonathansick jonathansick force-pushed the tickets/DM-15056 branch 2 times, most recently from 281047e to 40129fc Compare July 13, 2018 16:09
LTV1 and LTV2 no longer hold XY0 (those keywords are still written,
though we probably should not mention that, as LSST code never reads
them). XY0 is now encoded as WCS "A" using the following keywords:

CRPIX[12]A = 1
CRVAL[12]A = xy0
CTYPE[12]A = "LINEAR"
CUNIT[12]A = "PIXEL"

where [12] means 1 or 2, so CRPIX1 = CRPIX2 = 1, CRVAL1A = xy0[0],
CRVAL2A = xy0[1], etc.
This moves the Footprint.rst topic into the right place, and deletes the
older doc/detection directory.
No longer needed with stack-docs and package-docs command line tools.
@jonathansick jonathansick merged commit 218e8fb into master Jul 13, 2018
@ktlim ktlim deleted the tickets/DM-15056 branch August 25, 2018 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants