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-29242: Numpydoc conversion of meas_algorithms through psfSelectionFromMatchList.py #235

Merged
merged 1 commit into from May 21, 2021

Conversation

squisty
Copy link
Contributor

@squisty squisty commented Apr 14, 2021

No description provided.

Copy link
Contributor

@parejkoj parejkoj left a comment

Choose a reason for hiding this comment

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

This has been a useful reminder of how non-helpful many of our existing docstrings were!

There's an exclamation point that should go away in BasePsfDeterminerTask's docstring.

## @ref ObjectSizeStarSelectorTask_ "ObjectSizeStarSelectorTask"
## @copybrief ObjectSizeStarSelectorTask
## @}


@pexConfig.registerConfigurable("objectSize", sourceSelectorRegistry)
class ObjectSizeStarSelectorTask(BaseSourceSelectorTask):
r"""!A star selector that looks for a cluster of small objects in a size-magnitude plot
Copy link
Contributor

Choose a reason for hiding this comment

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

You forgot a few exclamation points!

And please add a period at the end.

@@ -227,7 +229,8 @@ def _kcenters(yvec, nCluster, useMedian=False, widthStdAllowed=0.15):


def _improveCluster(yvec, centers, clusterId, nsigma=2.0, nIteration=10, clusterNum=0, widthStdAllowed=0.15):
"""Improve our estimate of one of the clusters (clusterNum) by sigma-clipping around its median"""
"""Improve our estimate of one of the clusters (clusterNum) by sigma-clipping around its median
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a period here.

@@ -155,7 +156,8 @@ def __call__(self, ev):


def _assignClusters(yvec, centers):
"""Return a vector of centerIds based on their distance to the centers"""
"""Return a vector of centerIds based on their distance to the centers
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a period here.

@@ -122,7 +122,8 @@ def validate(self):


class EventHandler:
"""A class to handle key strokes with matplotlib displays"""
"""A class to handle key strokes with matplotlib displays
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a period here.

BasePsfDeterminerTask
#####################

``BasePsfDeterminerTask`` registers all PSF determiners with the `psfDeterminerRegistry`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see where you got that description from. Unfortunately, that comment is for people writing new PSF determiners, not the description of this one. Try this:

``BasePsfDeterminerTask`` is the base class for PSF determiners: override ``determinePsf`` to implement your algorithm, and register your new Task with ``psfDeterminerRegistry`` to allow it to be used in a pipeline.

ObjectSizeStarSelectorTask has a debug dictionary with the following boolean keys:

display
display = lsstDebug.Info(__name__).display
Copy link
Contributor

Choose a reason for hiding this comment

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

These keys should have the text from the doxygen here instead of the lsstDebug.Info line, e.g. bool; if True display debug information for this one.

PcaPsfDeterminerTask
####################

``PcaPsfDeterminerTask`` estimates psf based off of `measurePsfTask`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Try this:

``PcaPsfDeterminerTask`` estimates PSFs of pre-selected likely stars using a PCA decomposition approach.

----------
exposure : `lsst.afw.Exposure`
Exposure containing the psf candidates
psdCandidateList : `lsst.meas.algorithms.PsfCandidate`
Copy link
Contributor

Choose a reason for hiding this comment

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

Type of this is:

`list` [`lsst.meas.algorithms.PsfCandidate`]

detecting sources and then running them through a star
selector
metadata : `str`
A place to save interesting items
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and above, please end these descriptive sentences with a period (I know they were missing in the original).


Returns
-------
psf : `lsst.afw.detection.Psf
Copy link
Contributor

Choose a reason for hiding this comment

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

You're missing a backtick at the end, and the description should be "The fit PSF."

schema : `lsst.afw.table.Schema`
Schema used for sources; passing a schema allows the determiner
to reserve a flag field to mark stars used in PSF measurement,
but some PSF determiners ignore this argument.
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I forgot: the __init__ parameters should go in the class docstring, and __init__ has no docstring.

https://developer.lsst.io/python/numpydoc.html#placement-of-class-docstrings

@parejkoj
Copy link
Contributor

Thanks, this looks good. One comment above.

Can you please fix measureApCorrTask.rst on this ticket, so we don't forget it?

Here's the original text for that Task's display:
https://github.com/lsst/meas_algorithms/pull/226/files#diff-faf19ab47c73d530af818fb9a7595bf7e744af416ef11cf1007893c77e2a29caR290

And you can just reuse the text from the doPause parameter docstring:
https://github.com/lsst/meas_algorithms/pull/226/files#diff-faf19ab47c73d530af818fb9a7595bf7e744af416ef11cf1007893c77e2a29caR290

Copy link
Contributor

@parejkoj parejkoj left a comment

Choose a reason for hiding this comment

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

Thanks for all of this: the built docs look good.

Please rebase fixup to make one commit and rebase to master.

@squisty squisty merged commit 98a1dda into master May 21, 2021
@squisty squisty deleted the tickets/DM-29242 branch May 21, 2021 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants