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

applyDistortion does not propagate precision. #244

Open
Kelvinrr opened this issue Oct 25, 2019 · 9 comments
Open

applyDistortion does not propagate precision. #244

Kelvinrr opened this issue Oct 25, 2019 · 9 comments
Labels

Comments

@Kelvinrr
Copy link
Collaborator

applyDistortion in Distortion.cpp currently uses the default value for desirePrecision. ISIS3 uses pixel pitch which CSM does not have access too. We should probably propagate the desired ground precision and convert that to a precision in focal plane space.

@scsides
Copy link
Contributor

scsides commented Oct 25, 2019

Ground precision can very drastically within an image. Consider a flyby image where the whole disk is visible, or a wide angle camera with a FOV near 180deg. The distortion calculations only involve the focal plane why bring the ground into this? If we are going to propagate something why not the pixel pitch or use the coefficients for TransX,Y. These are usually the pixel pitch or very near to it.

@jessemapel
Copy link
Contributor

jessemapel commented Oct 28, 2019

A couple thoughts:

  1. Using a fixed fraction of the pixel pitch will probably work in most cases, but does not necessarily fit well with the CSM approach of requesting things within a user-provided precision. If we want to do this, the easiest way would be to compute the diagonal pixel size from the pixel2focal affine transformations. Taking the norm of the linear portion of the transformation (The determinant of the 2x2 matrix consisting of the second and third coefficients) will give us the diagonal pixel size.
  2. Converting the user-provided ground precision into a focal plane precision would fit better with how the CSM API works, but is problematic for all the reasons @scsides mentioned. For example, the pixel/m resolution of a disk resolved image varies is the lowest value at the point on the body closest to the sensor and then diverges to infinity as you approach the limb. Also, dynamically computing the ground resolution is too time consuming for a fundamental operation like groundToImage.

I think using a hard-coded percentage of the pixel pitch and using the ground resolution with the desired precision are a "six of one, half-dozen of the other". For that reason, I'd go with the simpler solution, the hard-coded percentage of the pixel pitch.

@oleg-alexandrov
Copy link
Collaborator

oleg-alexandrov commented Apr 29, 2022

I've noticed an issue with CTX data which is likely related to this. In the ground-to-image calculation, first an iterative scheme takes place which finds the correct line. No distortion is assumed. That scheme runs until the error goes below desired precision.

Then distortion is applied to the computed line and sample, another error is evaluated, and now the error is way above what was asked by the caller.

If this last error is the correct one, maybe the distortion operation should happen during the iterative scheme. I will attach below this a testcase (this has the model state, not the ISD).

B17_016219_1978_XN_17N282W.8bit.state.json.txt

@jessemapel
Copy link
Contributor

Adding iterative distortion inversion is a non-trivial amount of overhead. We could instead "portion" the precision so that we iterate until half the precision when doing the line search and then iterate until half the precision in the distortion inversion.

@oleg-alexandrov
Copy link
Collaborator

The testcase for this is now in the usgscsm repo, and is named tests/data/ctxLineScan.json.

@oleg-alexandrov
Copy link
Collaborator

@acpaquette : Is this related to #436?

@acpaquette
Copy link
Contributor

acpaquette commented Mar 2, 2023

@oleg-alexandrov That last comment from Jesse does seem related but the overhead seems negligible? Granted 5 seconds of overhead isn't negligible but does fix the problem we were seeing in KaguyaTC. I think coming back to this as a potential fix to speed up the fix applied in #436 would be a good idea

@oleg-alexandrov
Copy link
Collaborator

Ah, right. This original issue as filed was about propagation of precision, not modeling the distortion in ground-to-image, which is what you fixed.

@acpaquette
Copy link
Contributor

Right, and I think the case may be that the original change to the projective transform may be enough of a save that we can add the iterative distortion inverse in #436 and be about the same speed as before. It's hard to say. I think you are right though that this is related to the above discussion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants