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-11162: Replace all use of Coord and subclasses with SpherePoint #332
Conversation
d50bf30
to
596b759
Compare
Add getPosition and getTangentPlaneOffset methods from Coord to SpherePoint. Simplify the former by requiring the units argument and not treating hours specially. Fix documentation errors in the latter (it claimed the offset was to "this" but it's from "this").
Some exising users of Coord rely on a default constructor, so add one to SpherePoint.
596b759
to
62a913d
Compare
python/lsst/afw/coord/__init__.py
Outdated
from .utils import * | ||
from ._observatory import * | ||
from ._weather import * | ||
from ._weather import * |
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.
repeated line
src/geom/Endpoint.cc
Outdated
this->_assertNAxes(this->_getNAxes(data)); | ||
return coord::IcrsCoord(data[0] * radians, data[1] * radians); | ||
return SpherePoint(data[0] * radians, data[1] * radians); |
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.
Any reason not to use the SpherePoint(data[0], data[1], radians)
constructor here and below?
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.
Good idea. I'll do it.
src/geom/detail/frameSetUtils.cc
Outdated
#include "lsst/afw/fits.h" | ||
#include "lsst/afw/geom/Angle.h" | ||
#include "lsst/afw/geom/Point.h" | ||
#include "lsst/afw/geom/SpherePoint.h" |
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.
Does this file need to be included here?
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. Good catch.
62a913d
to
c646c3c
Compare
and related classes and functions, including CoordSystem, makeCoord, makeCoordEnum, and assertCoordsAlmostEqual
in test_warpExposure.py
The simple.cc pybind11 wrapper imports lsst.afw.coord. It should not be necessary, but removing it causes problems so I made a note with a reference to a possibly relevant RFC.
c646c3c
to
7de6782
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.
Looks good, only minor comments.
No description provided.