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

Fix bug in DenseAmpcor.py #217

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mzzhong
Copy link
Contributor

@mzzhong mzzhong commented Jan 5, 2021

This pull request for two purposes (1) fix a bug in DenseAmpcor.py, the script that calls the low-level fortran ampcor.F to generate (dense) offset fields and (2) add a simpler way of setting the location of chips given the parameters of chip and search window sizes.

(1) Fix the bug in DenseAmpcor.py
The bug I find is at line 419:
startDown, endDown = proc_loc_down[0], proc_loc_down[-1]
It should be:
startDown, endDown = proc_loc_down[0] - self.pixLocOffDn, proc_loc_down[-1] - self.pixLocOffDn

Some explanation for understanding the code and bug-fix:

  1. self.pixLocOffAc, self.pixLocOffDn are the half reference chip size + search window size in across and down direction (see line 647 and 648). The are used as the offset from the starting pixel of the secondary chip to the center of the reference/seconsary chip.

  2. self.gridLocAcross and self.gridLocDown store the the locations of the centers of all reference/secondary chips on the image in across and down directions. They are set at line 369 and 370: given the first and last valid pixel of the image (offAc, lastAc, offDn, lastDn), both self.pixLocOffAc and self.pixLocOffDn are used to determine the chip center.

  3. startAc, endAc, startDown, endDown are the starting pixel of the first and last secondary chips. They are passed to low-level ampcor.F. To set these four parameters:

At line 372:
startAc, endAc = offAc, self.gridLocAcross[-1] - self.pixLocOffAc
startAc and endAc are correctly set as the starting pixel of the secondary chip.

But at line 418-419:
proc_loc_down = self.gridLocDown[istart:iend]
startDown, endDown = proc_loc_down[0], proc_loc_down[-1]

startDown and endDown are set as the center of the reference/secondary chip by mistake.

It should be:
startDown, endDown = proc_loc_down[0] - self.pixLocOffDn, proc_loc_down[-1] - self.pixLocOffDn

  1. This code was correct in its original version. I find that it is correct in isce-2.1.0 (released in 2017). However, it is changed into this way in another version of isce2 in May 2018.

(2) Add a simpler way of setting the location of chips.
The current way of setting the location of chips is a bit complicated and difficult to understand. When developing GPU ampcor, we use a much simpler logic of determining this. I use limit_setting_option to determine which logic to use. The default logic is the conventional one (limit_setting_option=0). To turn on the new logic, user needs to set limit_setting_option manually to 1 in the code. I don't expose this as a configurable parameter because it will introduce unnecessary complexity to common users of ISCE, but would like to keep it there for future developers of GPU ampcor for benchmark purposes.

@rtburns-jpl rtburns-jpl requested a review from vbrancat January 27, 2021 21:35
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.

1 participant