-
Notifications
You must be signed in to change notification settings - Fork 21
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-26485: Add skyToPixelArray and pixelToSkyArray convenience functions. #538
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ | |
import astropy.coordinates | ||
import astropy.wcs | ||
import astshim as ast | ||
import numpy as np | ||
from numpy.testing import assert_allclose | ||
|
||
import lsst.utils.tests | ||
|
@@ -545,6 +546,56 @@ def testAgainstAstropyWcs(self): | |
# Most projections only seem to agree to within 1e-4 in the round trip test | ||
self.assertSkyWcsAstropyWcsAlmostEqual(skyWcs=skyWcs, astropyWcs=astropyWcs, bbox=bbox) | ||
|
||
def testPixelToSkyArray(self): | ||
"""Test the numpy-array version of pixelToSky | ||
""" | ||
cdMatrix = makeCdMatrix(scale=self.scale) | ||
wcs = makeSkyWcs(crpix=self.crpix, crval=self.crvalList[0], cdMatrix=cdMatrix) | ||
|
||
xPoints = np.array([0.0, 1000.0, 0.0, -1111.0]) | ||
yPoints = np.array([0.0, 0.0, 2000.0, -2222.0]) | ||
|
||
pixPointList = [lsst.geom.Point2D(x, y) for x, y in zip(xPoints, yPoints)] | ||
|
||
spherePoints = wcs.pixelToSky(pixPointList) | ||
|
||
ra, dec = wcs.pixelToSkyArray(xPoints, yPoints, degrees=False) | ||
for r, d, spherePoint in zip(ra, dec, spherePoints): | ||
self.assertAlmostEqual(r, spherePoint.getRa().asRadians()) | ||
self.assertAlmostEqual(d, spherePoint.getDec().asRadians()) | ||
|
||
ra, dec = wcs.pixelToSkyArray(xPoints, yPoints, degrees=True) | ||
for r, d, spherePoint in zip(ra, dec, spherePoints): | ||
self.assertAlmostEqual(r, spherePoint.getRa().asDegrees()) | ||
self.assertAlmostEqual(d, spherePoint.getDec().asDegrees()) | ||
|
||
def testSkyToPixelArray(self): | ||
"""Test the numpy-array version of skyToPixel | ||
""" | ||
cdMatrix = makeCdMatrix(scale=self.scale) | ||
wcs = makeSkyWcs(crpix=self.crpix, crval=self.crvalList[0], cdMatrix=cdMatrix) | ||
|
||
raPoints = np.array([3.92646679e-02, 3.59646622e+02, | ||
3.96489283e-02, 4.70419353e-01]) | ||
decPoints = np.array([44.9722155, 44.97167735, | ||
45.52775599, 44.3540619]) | ||
|
||
spherePointList = [lsst.geom.SpherePoint(ra*lsst.geom.degrees, | ||
dec*lsst.geom.degrees) | ||
for ra, dec in zip(raPoints, decPoints)] | ||
|
||
pixPoints = wcs.skyToPixel(spherePointList) | ||
|
||
x, y = wcs.skyToPixelArray(np.deg2rad(raPoints), np.deg2rad(decPoints)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you explicitly set degrees=False, future-proofing a change in the default value? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an interesting question. I see the point of explicitly setting this, but at the same time I think I'd like the test to fail if somebody inadvertently changes the default behavior and then they can explicitly update the tests if desired? E.g., it can be argued that tests should not be "future proofed" in certain ways. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, interesting. If someone, as a developer, changed the default behavior intentionally, they'd also fix the unit tests, but someone else, as a user, will find their code using these routines breaking. But if this is to protect against unintentional change of default behavior, I see your point. I'm okay with this staying as it is then. |
||
for x0, y0, pixPoint in zip(x, y, pixPoints): | ||
self.assertAlmostEqual(x0, pixPoint.getX()) | ||
self.assertAlmostEqual(y0, pixPoint.getY()) | ||
|
||
x, y = wcs.skyToPixelArray(raPoints, decPoints, degrees=True) | ||
for x0, y0, pixPoint in zip(x, y, pixPoints): | ||
self.assertAlmostEqual(x0, pixPoint.getX()) | ||
self.assertAlmostEqual(y0, pixPoint.getY()) | ||
|
||
def testStr(self): | ||
"""Test that we can get something coherent when printing a SkyWcs. | ||
""" | ||
|
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 think
ravel
is useful here. Ifxy
does not have exactly two rows, the call toapplyForward
will throw an error. And whenxy
has exactly two rows,ra
anddec
will be flat. Flattening isn't incorrect, but may just add to unnecessary calls without actually covering any edge cases.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.
Unfortunately, it seems that
vsplit
does not return 1d arrays but returns 2 2d arrays with shape[1, npoints]
. Theravel
call will flatten that without copying memory (unlikeflatten
which does a copy). In addition, aravel
on a true 1d array is essentially a no-op.