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-13950: Convert assembleCoadd.py to numpydoc #202

Merged
merged 1 commit into from May 23, 2018
Merged

Conversation

mrawls
Copy link
Contributor

@mrawls mrawls commented May 21, 2018

The scope of this ticket changed to update only assembleCoadd.py with numpydoc for now.

Copy link
Member

@jonathansick jonathansick left a comment

Choose a reason for hiding this comment

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

Whew, there's a lot here! Nice work.

I managed to get it to build after fixing a couple syntax issues I've pointed out.

In the assembleCoadd.py module, you'll want to add to config classes to __all__ so that they'll appear in the docs too.

__all__ = ["AssembleCoaddTask", "AssembleCoaddConfig", "SafeClipAssembleCoaddTask",
           "SafeClipAssembleCoaddConfig", "CompareWarpAssembleCoaddTask", "CompareWarpAssembleCoaddConfig"]

@@ -0,0 +1 @@
Copyright (c) 2008-2018 Association of Universities for Research in Astronomy, Inc.
Copy link
Member

Choose a reason for hiding this comment

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

This will need work (maybe create a new ticket if you can't do it here; it might be a bit more paper chasing than you want to take on!).

The design of the COPYRIGHT file is that each contributing institution has its own COPYRIGHT line, and the years that institution contributed meaningful work is recorded. https://developer.lsst.io/stack/package-docs.html#copyright

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see what happened. The assembleCoadd.py originally listed AURA/LSST as the copyright holder. That was probably wrong :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. Is this an acceptable interim option, or should I add a note saying the copyright file is incomplete or something? Happy to make a ticket for it.

Copy link
Member

Choose a reason for hiding this comment

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

I... wouldn't make a ticket just for this. I think it's understood that copyrights need to be systematically audited everywhere given the new system so there's gotta be an overarching ticket for all this anyways. I'll figure out the who and how of this work.

The `doMaskBrightObjects` and `brightObjectMaskName` configuration options
only set the bitplane config.brightObjectMaskName. To make this useful you
*must* also configure the flags.pixel algorithm, for example by adding
``config.measurement.plugins["base_PixelFlags"].masksFpCenter.append("BRIGHT_OBJECT")``
Copy link
Member

Choose a reason for hiding this comment

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

I think the original author intended this to become a code block:

*must* also configure the flags.pixel algorithm, for example by adding::

   config.measurement.plugins["base_PixelFlags"].masksFpCenter.append("BRIGHT_OBJECT")
   config.measurement.plugins["base_PixelFlags"].masksFpAnywhere.append("BRIGHT_OBJECT")

to your measureCoaddSources.py and forcedPhotCoadd.py config overrides.

with the flag '--legacyCoadd'. Usage of assembleCoadd.py expects
a data reference to the tract patch and filter to be coadded (specified
using
``'--id = [KEY=VALUE1[^VALUE2[^VALUE3...] [KEY=VALUE1[^VALUE2[^VALUE3...] ...]]')``
Copy link
Member

Choose a reason for hiding this comment

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

The code literal long enough to warrant a block:

using::

   '--id = [KEY=VALUE1[^VALUE2[^VALUE3...] [KEY=VALUE1[^VALUE2[^VALUE3...] ...]]')

[...]

This might require re-arranging the paragraph a bit.

@@ -12,3 +12,6 @@ Python API reference
====================

.. automodapi:: lsst.pipe.tasks
:skip: AssembleCoaddTask, SafeClipAssembleCoaddTask, CompareWarpAssembleCoaddTask
Copy link
Member

Choose a reason for hiding this comment

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

To get the API docs to show up, you'll need to change this to:

.. automodapi:: lsst.pipe.tasks.assembleCoadd

We'll generalize it to all of lsst.pipe.tasks when those APIs are available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Two questions here. (1) I was originally going to have just assembleCoadd listed, but it looks like @r-owen recently added all of lsst.pipe.tasks instead. Perhaps there's some special case I'm missing? (2) Does your comment here affect the :skip: line or just the .. automodapi:: line?

Copy link
Member

Choose a reason for hiding this comment

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

The comment is a replacement for both lines.

lsst.pipe.tasks doesn't import anything, turns out, so we need to point directly at individual modules

To begin, assuming that the lsst stack has been already set up, we must
set up the obs_subaru and ci_hsc packages. This defines the environment
variable $CI_HSC_DIR and points at the location of the package. The raw
HSC data live in the ``$CI_HSC_DIR/raw``h v directory. To begin assembling
Copy link
Member

Choose a reason for hiding this comment

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

This was generating a build error. Should be:

HSC data live in the ``$CI_HSC_DIR/raw`` directory. To begin assembling

@mrawls mrawls changed the title DM-13950: Finish pipe_tasks numpydoc conversion DM-13950: Convert assembleCoadd.py to numpydoc May 23, 2018
@mrawls mrawls merged commit 15fee6c into master May 23, 2018
@ktlim ktlim deleted the tickets/DM-13950 branch August 25, 2018 06:45
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