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-14133: Enable Sphinx support for meas_astrom #105
Conversation
Fix rst overline. Fix overline in package/index.rst Fix some numpydoc errors. Comment out doxygen for later conversion.
Add Task documention for AstrometryTask. Fix formating in AstrometryTask rst.
Fix links in docs. Touch up docs in display.py Clean up FitSipDistortion documentation. Fix rst heading. Change FitTanSip to numpydoc. Fix FitTanSipTask docs. Clean up setMatchDistance doc.
Fix task name.
Fix doc error.
Fix linter errors. Fix linting in astrometry.py
a6dd0d4
to
3d446a8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some style/consistency comments (including a lot of broken links), otherwise looks good.
.. toctree linking to topics related to using the module's APIs. | ||
|
||
.. .. toctree:: | ||
.. :maxdepth: 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any translation of the content from mainpage.dox. Admittedly, it's not much, but it should be preserved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm reading this wrong but shouldn't what is in mainpage.dox just be autogenerated under the different sections in this file? mainpage.dox just looks to be saying "here's AstrometryTask...". I've added a short sentence to the intro.
doc/lsst.meas.astrom/index.rst
Outdated
============ | ||
|
||
``lsst.meas.astrom`` is developed at https://github.com/lsst/meas_astrom. | ||
You can find Jira issues for this module under the `example <https://jira.lsstcorp.org/issues/?jql=project%20%3D%20DM%20AND%20component%20%3D%20example>`_ component. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bad URL, please replace example -> meas_astrom and check that it goes to the right Jira page.
================== | ||
|
||
.. If the task does not break work down into multiple steps, don't use a list. | ||
.. Instead, summarize the computation itself in a paragraph or two. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume these comments are from the template. Are they still necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm okay with keeping them in as they could be helpful is someone needs to update or expand the docs.
|
||
``FitSipDistortionTask`` involves three steps: | ||
|
||
- We set the CRVAL and CRPIX reference points to the mean positions of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think most of this needs to go into a "In Depth" section (see guidelines for Processing summary).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to leave this as is as it is merely a copy of what was in the doxygen.
|
||
.. lsst-task-config-fields:: lsst.meas.astrom.RefMatchTask | ||
|
||
.. _lsst.meas.astrom.RefMatchTask-debug: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file looks like it's been cut off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The doxygen for this class didn't some with any more examples or information so hence the lack sections in the rest of the file.
@@ -13,19 +13,20 @@ | |||
|
|||
|
|||
class MatchTolerance: | |||
""" Stores match tolerances for use in AstrometryTask and later | |||
iterations of the matcher. | |||
"""Stores match tolerances for use in `lsst.meas.astromAstrometryTask` and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: astrom.AstrometryTask
"""Test that an object is good for use in the WCS fitter. | ||
|
||
This is a hard coded version of the isGood flag from the old SourceInfo | ||
class that used to be part of this class. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You removed the old note about sourceSelector
not supporting a feature; is there an issue reference you could replace it with?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, there isn't. I don't think this the plan is to ever add this support as matchLists are different beasts than the source catalogs that source selectors work on. This was something I wrote a while ago when creating the new mathcer. Once I finalize the change over the matchers this becomes much less important and eventually goes away.
Rotation around the sphere | ||
|
||
Return | ||
------ | ||
float array | ||
Spherical unit vector x, y, z on the unit-sphere. | ||
output_array : `numpy.array`, (N, 4) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no such type as numpy.array
; did you mean numpy.ndarray
? Same elsewhere.
python/lsst/meas/astrom/ref_match.py
Outdated
A reference object loader object | ||
schema : `lsst.afw.table.Schema` | ||
ignored; available for compatibility with an older astrometry task | ||
kwargs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be **kwargs.
writes the distance field of each match | ||
Notes | ||
----- | ||
WARNING : the coord field of the source in each match must be correct |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could reproduce the old behavior with
WARNING : the coord field of the source in each match must be correct | |
.. warning:: | |
the coord field of the source in each match must be correct |
fe48e56
to
3d12e56
Compare
Correct numpy.ndarray types in docs. Fix typo. Fix doc formating. Update copyright header. Fix output name in doc. Change from cpp type to proper sphinx type. Removed cpp type in docs. Fix python preamble. Fix kwargs name. Fixup docs in astrometry and denormalize. Add sphinx reference and short meas_astrom description. Remove packMatches reference due to doc crash in that package. Remove duplicate reference. Rename matchOptimsiticB task docs. Add afw.table.packMatches back as unlinked. Fixup TODO comment.
8f5c68e
to
54dc09c
Compare
No description provided.