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-10973: Make SkyWcs transform to IcrsCoord instead of SpherePoint #247

Merged
merged 2 commits into from Jun 22, 2017

Conversation

r-owen
Copy link
Contributor

@r-owen r-owen commented Jun 20, 2017

No description provided.

virtual ndarray::Array<double, 2, 2> dataFromArray(Array const &arr) const override;

virtual Point pointFromData(std::vector<double> const &data) const override;
virtual int getNPoints(Array const &arr) const override;

Choose a reason for hiding this comment

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

Is Array::size private? Otherwise this seems like a pretty pointless (and confusing) function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because for GenericEndpoint.getNPoints Array is a 2-dimensional ndarray and so can't use size

@@ -55,6 +57,31 @@ def assertCoordsAlmostEqual(testCase, coord0, coord1, maxDiff=0.001*afwGeom.arcs


@lsst.utils.tests.inTestCase
def assertCoordListsAlmostEqual(testCase, coordlist0, coordlist1, maxDiff=0.001*afwGeom.arcseconds, msg=None):
"""!Assert that two lists of IcrsCoords represent almost the same point on the sky

Choose a reason for hiding this comment

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

Why the exclamation mark?

Copy link
Contributor Author

@r-owen r-owen Jun 20, 2017

Choose a reason for hiding this comment

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

So Doxygen recognizes it as Doxygen documentation

testCase.assertEqual(len(coordlist0), len(coordlist1), msg=msg)
sepArr = np.array([sp0.toIcrs().angularSeparation(sp1.toIcrs())
for sp0, sp1 in zip(coordlist0, coordlist1)])
badArr = sepArr > maxDiff

Choose a reason for hiding this comment

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

I assume angularSeparation is defined to be always positive. Probably is, but just checking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

@@ -132,6 +106,39 @@ Point2Endpoint::Point2Endpoint(int nAxes) : BaseVectorEndpoint<Point2D>(2) {
}
}

std::vector<double> Point2Endpoint::dataFromPoint(Point const& point) const {

Choose a reason for hiding this comment

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

Does it matter that it is now only defined for the derived class and no longer for the base class BaseVectorEndpoint<Point>?

Choose a reason for hiding this comment

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

If so, I think the comment @note Subclasses must provide arrayFromData should be updated to include dataFromPoint, dataFromArray and pointFromData.

@r-owen r-owen force-pushed the tickets/DM-10973 branch 3 times, most recently from 67f28bc to 119a2ae Compare June 21, 2017 21:45
Let clang-format 5.0.0 normalize end comments for namespaces
and a bit of comment indentation.
Also change `SkyWcs` to use `IcrsCoord` instead of `SpherePoint`.
@r-owen r-owen merged commit 45a5db4 into master Jun 22, 2017
@ktlim ktlim deleted the tickets/DM-10973 branch August 25, 2018 06:43
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