tickets/DM-15638: Fix DcrModel subfilter order. #101
Conversation
| if wcs.isFlipped: | ||
| cdAngle = (np.arctan2(-cd[0, 1], cd[0, 0]) + np.arctan2(cd[1, 0], cd[1, 1]))/2. | ||
| else: | ||
| cdAngle = (np.arctan2(cd[0, 1], -cd[0, 0]) + np.arctan2(cd[1, 0], cd[1, 1]))/2. |
There was a problem hiding this comment.
It took me a bit to distinguish the difference between these two lines, but I don't know of a cleaner way to do it.
Separately, do you have a ticket to lift this code out of here and into somewhere more generic (afw, maybe)? Other places would like to use it.
There was a problem hiding this comment.
I made a ticket, it is DM-16130.
tests/test_dcrModel.py
Outdated
| """Test that the bluest subfilter always has the largest amplitude. | ||
| """ | ||
| seed = 6 | ||
| rng = np.random.RandomState(seed) |
There was a problem hiding this comment.
Why these different random seeds? Why not just put one np.random.seed(5) in setUp and use np.random.X in the methods?
tests/test_dcrModel.py
Outdated
| @@ -134,14 +134,16 @@ def makeDummyWcs(self, rotAngle, pixelScale, crval): | |||
| Pixel scale of the projection. | |||
| crval : `lsst.afw.geom.SpherePoint` | |||
| Coordinates of the reference pixel of the wcs. | |||
| flipX : `bool`, optional | |||
| Optionally flip the direction of increasing Right Ascension. | |||
There was a problem hiding this comment.
Don't say "optionally flip": default True implies it defaults to being flipped, so it's not optional.
| afwGeom.Extent2D(0.3886592703, 0.2248919247)] | ||
| refShift = [afwGeom.Extent2D(-0.3103517169, -0.5363512808), | ||
| afwGeom.Extent2D(0.001092054612, 0.001887293861), | ||
| afwGeom.Extent2D(0.2248919247, 0.3886592703)] |
There was a problem hiding this comment.
Where did these numbers come from? A comment about what they are would be good.
There was a problem hiding this comment.
There's a comment at the start of the test, but I'll add another here.
tests/test_dcrModel.py
Outdated
| pixelScale = 0.2*arcseconds | ||
| rotAngle = 360.*rng.rand()*degrees | ||
| azimuth = 360.*rng.rand()*degrees | ||
| elevation = (45. + rng.rand()*40.)*degrees # Restrict to 45 < elevation < 85 degrees |
There was a problem hiding this comment.
Given that you set a random seed, this will always have the same value. Maybe lift the internals into a checkDcrSubFilterOrder method, and have a loop over some carefully chosen values (including zenith, north, south, east, west) in the testDcrSub... method that call check...?
tests/test_dcrModel.py
Outdated
| if dx < maskOffsetEnd: | ||
| refImage.mask.array[:, dx - maskOffsetEnd:] = maskValue | ||
| if dy < maskOffsetEnd: | ||
| refImage.mask.array[dy - maskOffsetEnd:, :] = maskValue |
There was a problem hiding this comment.
The above logic, with the dx/dy, and maskOffsetStart/End values defined just above it, suggests to me that you want to test a few different cases. Again, a checkDcrShift that this calls with a few different dx/dy and offsets seems like a good idea.
tests/test_dcrModel.py
Outdated
| rng = np.random.RandomState(seed) | ||
| refAngle = 0.*degrees | ||
| # Any additional arbitrary rotation should fall out of the calculation | ||
| cdRotAngle = 360*rng.rand()*degrees |
There was a problem hiding this comment.
This and the comment above definitely tell me there should be a loop over a range of cdRotAngles.
tests/test_dcrModel.py
Outdated
| def testRotationSouthZero(self): | ||
| """Test that an observation pointed due South has zero rotation angle. | ||
|
|
||
| An observation pointed due South should have zenith directly to the |
There was a problem hiding this comment.
Your test isn't of "due south", but rather "-30 deg, along the meridian".
tests/test_dcrModel.py
Outdated
| seed = 8 | ||
| rng = np.random.RandomState(seed) | ||
| # Any additional arbitrary rotation should fall out of the calculation | ||
| cdRotAngle = 360*rng.rand()*degrees |
There was a problem hiding this comment.
As above, test a range of rotAngles.
tests/test_dcrModel.py
Outdated
| self.assertAnglesAlmostEqual(-cdRotAngle, rotAngle, maxDiff=1e-6*radians) | ||
|
|
||
| def testRotationNotFlipped(self): | ||
| """Check the interpretation of rotations in the WCS. |
There was a problem hiding this comment.
This and testRotationFlipped could be factored out into checkRotation that takes a flipX parameter and tests against the negative cdRotAngle if True. Then you can check a range of angles both flipped and not using the same code block.
|
Thanks for updating the tests. I still think |
isullivan
force-pushed
the
tickets/DM-15638
branch
from
cf966f6
to
b818144
Compare
A few lines of code were changed, and several unit tests were added.
The order of the DrModel subfilters was reversed, which was due to the calculated shift due to DCR having the wrong sign. There were several bugs that worked together in this: the rotation calculation was written for a flipped WCS, but if a non-flipped WCS was supplied it would be off by -90 degrees. Then,
afwGeom.Extent2D()takes arguments in the order X,Y but had been supplied in the order Y,X, so that had been off by another 90 degrees. Finally, DCR is mostly symmetric around the center of the band, so everything still mostly worked even though the shift in each band had the wrong sign.