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

updated distortion to closer match ISIS #243

Merged
merged 3 commits into from
Oct 28, 2019
Merged

Conversation

Kelvinrr
Copy link
Collaborator

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.

@Kelvinrr Kelvinrr requested a review from jessemapel October 25, 2019 22:06
@@ -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);
Copy link
Contributor

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.

@scsides scsides self-assigned this Oct 28, 2019
Copy link
Contributor

@scsides scsides left a comment

Choose a reason for hiding this comment

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

What Jesse said

@krlberry
Copy link
Contributor

Like Stuart, I also agree with what Jesse said.

@Kelvinrr
Copy link
Collaborator Author

For now this is basically a bug fix, we should probably talk more about how to properly solve this problem here: #244

Copy link
Contributor

@jessemapel jessemapel left a 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.

@jessemapel jessemapel merged commit 8c980d2 into DOI-USGS:dev Oct 28, 2019
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.

4 participants