-
Notifications
You must be signed in to change notification settings - Fork 35
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
updated distortion to closer match ISIS #243
Conversation
@@ -30,5 +30,5 @@ void removeDistortion(double dx, double dy, double &ux, double &uy, | |||
void applyDistortion(double ux, double uy, double &dx, double &dy, | |||
const std::vector<double> opticalDistCoeffs, | |||
DistortionType distortionType, | |||
const double desiredPrecision = 0, const double tolerance = 1.0E-6); | |||
const double desiredPrecision = 1.0E-6, const double tolerance = 1.0E-6); |
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 would make this one argument, the tolerance. The desired precision should be handled by the model that calls this and converted into a convergence tolerance.
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.
What Jesse said
Like Stuart, I also agree with what Jesse said. |
For now this is basically a bug fix, we should probably talk more about how to properly solve this problem here: #244 |
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 to fix the bug with passing 0 tolerance. Further changes to how that tolerance is determined can be handled in another PR.
Increased max iteration count, loosened precision, and acceptance criteria for Radial Distortion.
Isis uses pixel pitch as the tolerance, CSM doesn't have access to pixel pitch so just punting it for now.