Fix corner case crash with parse_match_file.py

PicoJr commented 2 years ago

Description

When there is exactly one IP record in the match file the script parse_match_file.py fails.

Explanation: np.genfromtxt returns a tuple instead of an array cf https://stackoverflow.com/questions/24429822/genfromtxt-generates-tuples-so-does-recfromcsv.

This is problematic: the scripts fails with TypeError: len() of unsized object when calling len(im1_ip) (same with len(im2_ip).

This PR reshapes the result so that it is always an array => prevents failure.

Motivation and Context

This PR prevents a crash occurring when calling parse_match_file.py on a match file with a single IP Record for both or one image.

How Has This Been Tested?

test.bin.txt

1 2
42.0 43.0 42 43 52.0 53.0 54.0 3 62 63 2 44.0 44.0
42.0 43.0 42 43 52.0 53.0 54.0 3 62 63 2 44.0 44.0
42.0 43.0 42 43 52.0 53.0 54.0 3 62 63 2 44.0 44.0

Without this PR

python parse_match_file.py -rev test.bin.txt test.bin.out

output:

Reading: test.bin.txt
Writing: test.bin.out
Traceback (most recent call last):
  File "/home/redacted/clones/StereoPipeline/src/asp/Tools/parse_match_file.py", line 199, in <module>
    write_match_file(args.outfile, im1_ipb, im2_ipb)
  File "/home/redacted/clones/StereoPipeline/src/asp/Tools/parse_match_file.py", line 121, in write_match_file
    size1 = len(im1_ip)
TypeError: len() of unsized object

With this PR

python parse_match_file.py -rev test.bin.txt test.bin.out

output:

Reading: test.bin.txt
Writing: test.bin.out

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

[ ] I have added tests to cover my changes.
[ ] All new and existing tests passed.

Licensing:

This project is released under the LICENSE.

I dedicate any and all copyright interest in my contributions in this pull request to the public domain. I make this dedication for the benefit of the public at large and to the detriment of my heirs and successors. I intend this dedication to be an overt act of relinquishment in perpetuity of all present and future rights to this contribution under copyright law.

oleg-alexandrov commented 2 years ago

This is a very good catch, thank you!

Yet, I hope you won't feel slighted if I say that I would rather not add your name to the list of authors. Bugfixes are very valuable, and they make a difference between a great package and a no-good package, but the bar for being included in the list of authors is higher than a bugfix. We receive a lot of valuable feedback from the community, via bug fixes, feature requests, bug reports, questions, documentation suggestions, and while all these result in improvements, most of the time they go unacknowledged as otherwise the list of authors would be too big to have any meaning.

If that is fine with you, I will pull only the fix itself.

PicoJr commented 2 years ago

This is a very good catch, thank you!

Yet, I hope you won't feel slighted if I say that I would rather not add your name to the list of authors. Bugfixes are very valuable, and they make a difference between a great package and a no-good package, but the bar for being included in the list of authors is higher than a bugfix. We receive a lot of valuable feedback from the community, via bug fixes, feature requests, bug reports, questions, documentation suggestions, and while all these result in improvements, most of the time they go unacknowledged as otherwise the list of authors would be too big to have any meaning.

If that is fine with you, I will pull only the fix itself.

Sure, I don't mind, please pull the fix =)

(edit): I opened the corresponding issue #349

oleg-alexandrov commented 2 years ago

Thank you! I did add your name in a comment in the tool itself. Credit is important. https://github.com/NeoGeographyToolkit/StereoPipeline/commit/a9133ac4267656f18447a9f057ca27b5010386a8#diff-112b3306a839cf78e7d89272b49252653c4f697d6c66d89ba2e38335b3faaa52

This tool has another quirk I hope to fix one day. It writes the descriptors to text, but does not bother to read them from text, so the tool is not quite reversible. Maybe one day.

NeoGeographyToolkit / StereoPipeline