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-14275: The distortion in test_wcsUtils.py testDistortion is unreasonable #350

Merged
merged 2 commits into from May 2, 2018

Conversation

r-owen
Copy link
Contributor

@r-owen r-owen commented May 2, 2018

No description provided.

r-owen added 2 commits May 1, 2018 21:09
Increase the number of iterations for the inverse of radial
distortion models. Normally only a few are needed, but in some
cases it helps to have more iterations.
Fixed two errors in test_wcsUtils.py that resulted in unreasonable
values:
- The focalPlaneToPixel transformation was backwards
- The focalPlaneToFieldAngle computed in testDistortion was incorrect
Copy link
Contributor

@parejkoj parejkoj left a comment

Choose a reason for hiding this comment

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

One question about speed.

Separately, why was the transform backwards before?

@@ -124,7 +124,7 @@ ast::PolyMap makeOneDDistortion(std::vector<double> const &coeffs) {
polyCoeffs[i - 1][2] = i;
}

return ast::PolyMap(polyCoeffs, 1, "IterInverse=1, TolInverse=1e-8, NIterInverse=20");
return ast::PolyMap(polyCoeffs, 1, "IterInverse=1, TolInverse=1e-8, NIterInverse=30");
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this runs 30 steps every time it computes the inverse? How much slower is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NIterInverse is an upper limit. For most transforms the solution will reach the tolerance specified by TolInverse long before it hits the limit on iterations and stop. However, our unit tests have found several examples where 20 is not enough and so the results are silently lousy. In each such case 30 has been more than enough. It's not a systematic approach, but 30 is clearly better than 20 for accuracy.

focalPlaneToFieldAngle = afwGeom.makeRadialTransform([0.0, self.radPerMm, 0.0, self.radPerMm])
# Compute a distorted wcs that matches self.tanWcs at the center of the field;
# the amount of distortion is 10s of pixels over the detector
fieldAngleToFocalPlane = afwGeom.makeRadialTransform([0.0, 1/self.radPerMm, 0.0, 1000/self.radPerMm])
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah the joys of picking a "reasonable" fake model. This seems like a better choice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It gives much more reasonable values, and is very similar to what LSST uses (the factor of 1000 is not in the LSST model, but the LSST model only shows significant distortion at much larger focal plane radius; I tuned it to get the distortion I wanted over the range of values this test uses).

@r-owen r-owen merged commit a96283e into master May 2, 2018
@r-owen
Copy link
Contributor Author

r-owen commented May 2, 2018

The transform was backwards before because I forgot to check transformed values to see if they looked reasonable. The test passed, so I didn't worry about it. I suppose I could have left it alone, but it sure makes debugging easier if the code does what it says it does.

@ktlim ktlim deleted the tickets/DM-14275 branch August 25, 2018 06:44
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