search

DOI-USGS / usgscsm

This repository stores USGS Community Sensor Model (CSM) camera models
Other
26 stars 33 forks source link

Partial fix to the issue for overlap among the LRO WAC framelets #397

Closed oleg-alexandrov closed 2 years ago

oleg-alexandrov commented 2 years ago

This is an incremental improvement upon the issues noted in https://github.com/USGS-Astrogeology/ISIS3/issues/4944 and https://github.com/USGS-Astrogeology/ISIS3/issues/4945. Each WAC framelet has the same data in its last several rows as the earliest rows in the next framelet. Moreover, some of those rows have pixel values flagged as invalid (null) even though they look reasonably valid.

The proposed fix wipes several top and bottom lines in each framelet. This is handled correctly both in the CSM camera model logic and the framestitch application (see link below). The total number of wiped lines in a frame is hard-coded to 2 in the PushFrame camera implementation. It will be parameter in the framestitch tool, with a default value of 2.

A side effect of this fix is that since there's less redundancy among the frames, the image-to-ground and ground-to-image functions are more likely to be the inverse of each other. To validate that, run before and after this fix the command:

usgscsm_cam_test --model ../tests/data/lroWacPushFrame.json --sample-rate 1000 --desired-precision 1e-10

The max discrepancy between a pixel going to the ground and then coming back to the camera before the fix is about 6 pixels, and goes down to 4 pixels after it. The testcase above will be committed to the repo as part of this pull request. Its original name is M119923055ME.json.

See for more details the framestitch pull request in the ISIS repo at https://github.com/USGS-Astrogeology/ISIS3/pull/4950.

oleg-alexandrov commented 2 years ago

I added a small commit to this pull request having only some comments and notation tune-up. I also tried to optimize the Lagrange interpolation, which takes on the order of perhaps 40% of the time for ground-to-image computation, but that was not successful. Presumably the compiler is clever about computing those repeated multiplications I tried to compute just once. Hence I did not add that change to the code, but added a comment to note this.

oleg-alexandrov commented 2 years ago

I added a minor bugfix for installation to this pull request. The "spdlog" directory was being installed in "include/include/spdlog" and could not be found. Then there were duplicated header files, installed in "include/usgscsm" which is correct, but also in "include/include/usgscsm" which is not.

jessemapel commented 2 years ago

Working on reviewing this and the other PR right now

jessemapel commented 2 years ago

I'm going to hold off on posting review comments here until the ISIS PR is resolved as it will have a bunch of knock-on effects here:

https://github.com/USGS-Astrogeology/ISIS3/pull/4950

oleg-alexandrov commented 2 years ago

I will look at your comments in the ISIS repo pull request.

Here I put another fix. USGSCSM should ship only its own things, not Eigen as well. The line which I removed with this commit was problematic. When built and installed with conda, it was installing, for some reason, a file called "eigen3" where there should be a directory with this name.

I could not track what is going on, and likely it is better for this package to not mess up copying Eigen header files on install. If Eigen is needed, conda will fetch it and install it from conda-forge.

This was tested on Linux and OSX.

oleg-alexandrov commented 2 years ago

I added the ability to read num_lines_overlap from JSON file if present. I tested it and it works. The only problem left is how to trick framestitch to create a cub file which will save this and also have valid kernels and other metadata inherited from the parent cubes.