Fix findfeatures matrix inversion issues and improve FastGeom performance #4772
Conversation
* Fix matrix inversion error on empty matrix. (Fixes DOI-USGS#4639) * Identify images that fail FastGeom transform and exclude from matching. * Add TONOGEOM parameter to write failed file loads or FastGeom error file list if the transform cannot be determined * Improve FastGeom transform algorithm using new radial point mapping scheme * Add more debugging output to help diagnose problem images/procedures
* findfeatures mod to add Grid algorithm to FastGeom class * Improved Grid algorithm by computing proper starting iteration to statisfy fastgeompoints request * Added additional parameterization of Grid algorithm * Reorganized mapping process to consolidate Radial and Grid algorithms in FastGeom
There was a problem hiding this comment.
Overall looks good. I have a few questions about constant choices and what the desired matching patterns are. Most of the comments are just readability and clean-up.
The point distribution and matching algorithm is quite complex, it would be really helpful to separate it out so that it can be unit tested more easily.
|
I am currently testing these modifications and will be ready to push them to the branch tomorrow unless you have additional comments/concerns. |
findfeatures modifications: * Refactored FastGeom separating large sections of the compute() methods * Redesigned the the radial algorithm for easier user configuration * Added sanity checks avoid bad geometric correspondences * Added GLOBALS parameters for easy configuration of FastGeom algorithms at runtime
…sBecker/ISIS3 into findfeatures_matrix_fix_mods
|
Thanks @jessemapel for your very helpful feedback. Your comments compelled a deeper dive in testing and evaluation of the modifications. This identified additional issues and errors that produced a rather large refactor. I pushed those updates into my branch that is ready for review. Clearly this app begs fo an overarching guide to help make it easier to use effectively and productively. Its something on my todo list for sure. Please let me know if you have any other concerns. |
|
Code looks good. For testing, I think the best way to handle this is to have simple tests for findfeatures that ensure it's calling the correct algorithm between grid and ring. Then, the grid and ring algorithms can be unittested to ensure that all of the parameters are being passed in and used correctly. |
|
@KrisBecker @jessemapel What is the status on this? Looks like it is waiting on tests? |
|
Correct this just needs tests and it's ready to go |
|
Thank you for your contribution! Unfortunately, this pull request hasn't received much attention lately, so it is labeled as 'stale.' If no additional action is taken, this pull request will be automatically closed in 180 days. |
github-actions
bot
added
the
inactive
|
This pull request is still active and should remain open. Thanks! |
github-actions
bot
removed
the
inactive
The ThreeImageNetwork.FunctionalTestFindfeaturesErrorNoInput expects an error to be thrown with spectific text. The text in findfeatures.cpp was changed in this PR. It fixes the existing findfeatures test suite.
…sBecker/ISIS3 into findfeatures_matrix_fix_mods
The cnetwinnow test needs tempDir().path() prepended to the “file_prefix” parameter to properly place the output files in the teardown directory.
|
Ok, I think these modifications address the remaining issues. Could someone please review this. Thanks for your patience. |
|
Pinging this PR for review/merge. Thanks... |
|
@KrisBecker Do you have an idea when some of the above discussion will be added to this PR? |
|
I am currently working on this along with other distractions. I am going to be on vacation later this week into next week so it will be at least two weeks away. If you are planning a release soon, would it be possible to merge this PR and I submit another one when I have completed this work? Thanks... |
|
@KrisBecker I think we can make that happen, I will open another issue to get docs on this once we merge this PR |
|
@acpaquette that would be awesome! Thank you! |
|
@KrisBecker I'm in the process of peeling a release at the moment, and I'm in the final stages of merging PRs so that they can be included in 8.0.0 LTS. I brought this PR up during our standup, and the team is somewhat on the fence about merging this without addressing some of the comments in the most recent review, particularly with respect to missing documentation. That being said, I recognize that it's important to get this merged. I can delay the release build until tomorrow morning, but any later than that would make me worried about the release timeline. If there's anything that I can do to help move this along before then, please let me know, and I'll do my best to help so that this can be included in the release-in-progress. |
…sBecker/ISIS3 into findfeatures_matrix_fix_mods
- Improved reporting of parameterizations of findfeatures - Prevent creation of empty TONOTMATCH file when none are detected
- Added two new examples demonstrating/documenting the use of FASTGEOM algorithm, parameterization using GLOBALS and how to produce a regional mosaic using findfeatures with batch scripts. - Reviewed, clarified and improved findfeatures program documentation
|
Thank you for your patience. I have updated the documentation and added some additional examples that addresses concerns. Tables have been added the describes all the parameters that can be applied to modify findfeatures behavior. The examples demonstrate use this feature. Please review and let me know if anything remains to get this PR merged. Thanks for all your suggestions and help with this. |
|
I think going forward we need to split these sort of PRs into smaller more manageable chunks. Large PR's almost guarantee this sort of slow progress in the merge process as it's more like merging a large feature branch. For now we still need to focus on wrapping this one up before or during the next support sprint. |
|
I agree this one got much more difficult over time. If you think it would help we can arrange for an interactive review. This might help it move along quicker toward merge. I do see a few typos that I should fix. I may need to restructure the change log entry (its all over the place), so if you have any suggestions there that would be great. Let me know how I can help get this merged. Thanks... |
There was a problem hiding this comment.
Hi @KrisBecker, the documentation provided looks good! Couple of things to change here and there. I am not sure about the control network generation tutorial being housed under findfeatures template section as well as the tutorial being placed in the examples of the findfeatures app.
I think a better place to put the tutorial scripts is isis/scripts/findfeatures_mosaic_tutorial/, then we can add a new tutorial page under the ISIS wiki in the advanced section called something like "Making Mosaics Using findfeatures"
- Modified findfeatures.xml documentation to address PR review feedback - Fixed use of projected images which wasn’t working due to improper instantiation of the cube projection object - Updated CHANGELOG.md to better categorize all changes in this PR
|
I believe all the comments/concerns have been addressed. Please let me know if there is anything left to do se we can get this PR merged. Thanks... |
@KrisBecker When I said ping the reviewer, I meant to @ them so they get pinged. Adam has been busy putting out some fires so I'm gonna take over review work with his permission. I agree it's good to go except for the scripts which is the odd man out. |
There was a problem hiding this comment.
I think this is good except we should remove those tutorial scripts. Or someone needs to make a convincing argument that it's fine.
They can be reintegrated later down the line. My desire to see this merged is only slightly lower than my desire to avoid introducing more tech debt.
CHANGELOG.md
Outdated
| - Add new program optiom <b>TONOGEOM</b> that logs captures geometry errors in the FASTGEOM algorithm and records them to the file provided in this parameter. These images are excluded from the matching process. References [#4772](https://github.com/DOI-USGS/ISIS3/issues/4772) | ||
| - Added a new Radial FASTGEOM transform mapping algorithm to address performance problems with the Grid algorithm. This is now the default algorithm none are selected by the user (see the new <b>GLOBALS</b> parameter to specify the algorithm) [#4772](https://github.com/DOI-USGS/ISIS3/issues/4772) | ||
| - Added new parameter <b>GLOBALS</b> to make parameterization of <i>findfeatures</i> behavior significantly easier and convenient. Wrote significant documentation for the parameter and provide several examples showing its use. [#4772](https://github.com/DOI-USGS/ISIS3/issues/4772) | ||
| - Added two new examples demonstrating/documenting the use of FASTGEOM algorithm, parameterization using <b>GLOBALS</b> and how to produce a regional mosaic using <i>findfeatures</i> with batch scripts. [#4772](https://github.com/DOI-USGS/ISIS3/issues/4772) | ||
| - Added new option <b>GEOMSOURCE=BOTH</b> to check both the MATCH and FROM/FROMLIST images for valid control measure geometry to produce better networks and prevent downstream processing errors. Ignore points that end up with no valid measures (but can retained with use of <b>PreserveIgnoredControl</b> via GLOBALS parameterization). [#4772](https://github.com/DOI-USGS/ISIS3/issues/4772) | ||
| - Added new gtests for <i>findfeatures</i> that replaces all the old application tests. These tests are <i>FunctionalTestFindfeaturesFastGeomDefault</i>, <i>FunctionalTestFindfeaturesFastGeomRadialConfig</i>, <i>FunctionalTestFindfeaturesFastGeomGridDefault</i> and <i>FunctionalTestFindfeaturesFastGeomGridConfig</i>. [#4772](https://github.com/DOI-USGS/ISIS3/issues/4772) |
There was a problem hiding this comment.
As per our discussion at the TC meeting. The fact that his required multiple changelog lines I believe indicates that this should have been split up into multiple PRs.
There was a problem hiding this comment.
I agree this PR became much larger than I expected or even imagined. However, the scope was identified and explained in issue #4772 and in the context of this PR on Feb 3, 2022. At no time did anyone mention this concern even after the initial commits were made.
Clearly, the review process (you are now the third reviewer) has contributed greatly to the size of this PR. Its also clear it did get much larger than anyone expected. I also learned a lot about this process and will be mindful to take steps to prevent this from occurring again. For one thing, I will not let my PRs linger so long between reviews - definitely did not help at all.
I am grateful for the reviews as they have been very helpful and useful to make this PR better. The unfortunate consequence of this is it also made it much larger. Much of the size was in response to requests for documentation improvements/clarifications, which is always useful.
I am open for suggestions/discussion about how this PR could be broken up or made easier and more efficient somehow. Hindsight would indicate to not address two issues in one PR even if you think they are small and/or related.
There was a problem hiding this comment.
I think at this point, it's too late to break up this PR. It has been through the review process and has had pretty much everything already addressed. But something to consider as a policy to avoid it in the future. Something for the ISIS TC.
Once the merge conflicts are resolved and the scripts are removed, I'll approve and merge this
CHANGELOG.md
Outdated
| - Fix matrix inversion errors in findfeatures due to bad FASTGEOM matrix transforms using a more robust implementation to detect these errors and throw exceptions. Images with these errors are captured and logged to the <b>TONOTMATCHED</b> file. Fixes [#4639](https://github.com/DOI-USGS/ISIS3/issues/4639) | ||
| - Fixed use of projected mosaics with correct check for <b>TargetName</b> in the Mapping labels. [#4772](https://github.com/DOI-USGS/ISIS3/issues/4772) | ||
| - Fixed a bug in the <i>cnetwinnow</i> test that did not clean/remove it during test runs. | ||
| - Fixed instantiation and use of projection classes to correctly return geometry data from projected images and mosaics. [#4772](https://github.com/DOI-USGS/ISIS3/issues/4772) | ||
|
|
There was a problem hiding this comment.
Even further evidenced that this is a mix of both Bugfixes and feature adds that seem to affect multiple issues. This would probably make for a good policy discussion.
There was a problem hiding this comment.
The only bug fix in the above entries that was outside the scope of this PR is line 62. It was something I discovered during testing and was fixed in the testing framework.
- Per request via USGS review, removed the Bash shell scripts that produce the results of example 4 - Removed the $ISISROOT/appdata/templates/findfeatures/mosaics containing the scripts - Updated documentation in findfeatures.xml accordingly - Updated CHANGELOG.md accordingly
…x_mods # Conflicts: # CHANGELOG.md
|
@Kelvinrr updates have been completed and conflict resolved. Let me know if there is anything remaining. Thanks... |
github-actions
bot
added
inactive
findfeatures matrix inversion issues are addressed in this PR.
Description
Matrix inversion errors have been reported in #4639 that cause findfeatures to abort. This appears to be directly related to problems with the FastGeom procedure. Images that have very little to no overlapping common coverage in their FOVs are typically the source of problems in the ImageSource class.
This PR address the matrix inversion problem with better detection of an invalid matrix. Also, improvements to the FastGeom algorithm were required to detect and minimize the impact on successful use of lists of images, particularly.
The FastGeom process was implemented in the ImageSource class that made corrections to the algorithm problematic because it lacks parameterization/control of the grid-based geometric mapping correspondence algorithm. Therefore, this algorithm, now referrred to as
Grid, was reimplemented directly in the FastGeom class. This is an iterative algorithm to that will increase the density of the candidate geometry points in the MATCH image on each successive iteration and run unbounded until every pixel was compared and still failed.An additional FastGeom algorithm, called
Radial, was implemented to compute radial patterns of corresponding geometry patterns with increasing point density on each circle. This algorithm is not iterative but can be configured to produce varying density and radial spacing outward from the center of the image.As a consequence, these image failures are more quickly and efficiently identified and removed from the matcher list and optionally reported in a user specified file that contains a list of images that cannot be transformed or the matrix inversion fails. Hence, a new parameter to findfeatures was added called TONOGEOM that contains this list.
In addition, a known problem with construction of control points and measures is also addressed in this PR. At times, only the MATCH geometry control measure coordinate was set in the newly created control point. However, the corresponding geometry control measure in the FROM/FROMLIST images was never checked/validated. This caused downstream processing problems, particularly in jigsaw.
Lots of debugging information has been added to help in better understanding and track performance of FastGeom and creation of the output control network.
Here is the summary of changes contained in this PR:
Related Issue
Fixes #4639.
Fixes #4771.
Motivation and Context
findfeatures would abort on single images when applying FastGeom processing to computed a warp transform to better spatially compare the MATCH image to FROM/FROMLIST images. This allows use of OpenCV feature/detector algorithms that are not rotation and/or scale invariant.
Problems with a single image would abort findfeatures when they could simply be excluded from the matching process and continue processing images that are successfully processed.
How Has This Been Tested?
These changes have been tested in for several months processing NEAR/MSI Eros and Rosetta/OSIRIS-NAC 67P images. Modifications to findfeatures affect only code used in the application and does not impact any other system classes or functionality.
Screenshots (if appropriate):
Types of changes
Checklist:
Licensing
This project is mostly composed of free and unencumbered software released into the public domain, and we are unlikely to accept contributions that are not also released into the public domain. Somewhere near the top of each file should have these words: