Fix the Ampcor issues from Lijun's branch and increase the limit of the search radius #282
Conversation
…o fix_ampcor_issue1
xhuang-jpl
changed the title
|
@lijun99 Thanks again for your PR, would you mind taking a look the merge? It seems that I cannot add you to be the reviewer. |
|
I tested the merged version and it works fine. Thanks @xhuang-jpl ! |
Thanks @lijun99 , I also tested, and works fine on both CPU and GPU. |
hfattahi
left a comment
hfattahi
left a comment
There was a problem hiding this comment.
The bug fixes look reasonable to me. thanks @lijun99 and @xhuang-jpl .
|
@xhuang-jpl please update the PR description to clarify that you have also relaxed the schema and allow larger half search window. |
xhuang-jpl
changed the title
| if(referenceStartPixelDown[i] < 0) | ||
| fprintf(stderr, "Warning: Reference Window start pixel out of range in Down, window (%d,%d), pixel %d\n", row, col, referenceStartPixelDown[i]); | ||
| if(referenceStartPixelAcross[i] < 0) | ||
| fprintf(stderr, "Warning: Reference Window start pixel out of range in Across, window (%d,%d), pixel %d\n", row, col, referenceStartPixelAcross[i]); |
There was a problem hiding this comment.
It looks like the previous behavior was to raise an exception when this check failed. The PR changes that behavior to emit a warning message instead. Could you explain the rationale behind this change?
There was a problem hiding this comment.
Good point @seongsujeong , it is because they are not the issue anymore, and just warn the user that the window might out of range. @lijun99 has done the fix via filling 0. Please check his reply:
The isce3 version uses a strict range check, which could fail when we choose a specific starting pixel and some pixels from search image may fall out of range (at the edges). I have patched the code from the standalone version, which fills (0,0) when using pixels out of range. Now checkPixelInRange is optional.
There was a problem hiding this comment.
In Image loading/copying, zeros are now filled for out of range pixels (happening at edges if margins are not set properly or if we want to use a fixed start pixel location and vary chip sizes or search ranges). Of course, if it happens, the results along the edge are not reliable and therefore warnings may be issued. But it should not prevent the program from running.
Before the revision, accessing out of range pixels leads to memory access errors (seg fault, at least for CUDA), and therefore checking the range and raising an exception was the only plausible solution.
Note also that the above bug fixes have been included in the PyCuAmpcor V2 PR. Somehow, I forgot to patch V1. So, current version of ISCE2 has similar issues.
| int idxOut = idxImage*outNX*outNY + outx*outNY + outy; | ||
| int idxIn = (offsetX[idxImage]+outx)*inNY + offsetY[idxImage] + outy; | ||
| imageOut[idxOut] = make_float2(complexAbs(imageIn[idxIn]), 0.0); | ||
| int inx = offsetX[idxImage] + outx; |
There was a problem hiding this comment.
Here is an example that range is checked during copying and zeros are filled when out of range happens.
seongsujeong
left a comment
seongsujeong
left a comment
There was a problem hiding this comment.
After the discussion with @xhuang-jpl offline, I am going to approve the PR because it is well tested with various test dataset.
commit 0e1f80c Author: xhuang-jpl <118782850+xhuang-jpl@users.noreply.github.com> Date: Wed Jun 24 16:38:22 2026 -0700 Fix the Ampcor issues from Lijun's branch and increase the limit of the search radius (#282) * sm r3.4 * change the SM commit id for R4.0.2 * remove the mask description for the wrapped ifgram * remove the tmp folder * fix minors * comment out the checkPixelInImageRange * pycuampcor: fix the pixels out of range issue * pycuampcor: fix an error in determining the corrwindow size * update the maximum search radius to be 256 --------- Co-authored-by: Xiaodong Huang <xhuang@nisar-adt-dev-3.jpl.nasa.gov> Co-authored-by: Lijun Zhu <ljzhu@gps.caltech.edu> commit 813d729 Author: Brian Hawkins <1729052+bhawkins@users.noreply.github.com> Date: Wed Jun 24 13:56:09 2026 -0500 Handle case where no samples in a rangline are valid (#327) * Handle case where all subswaths are invalid. * Log some details about missing data. * Fix typo * Use signed integer for negative values... commit 075795f Author: Gustavo H. X. Shiroma <52007211+gshiroma@users.noreply.github.com> Date: Thu Jun 18 15:59:58 2026 -0700 Fix STATIC filenames with fractional posting by rounding posting after converting to decimeters. (#314) * disable polarimetric symmetrization by default * revert changes to `symmetrize_cross_pol_channels` * Update GCOV and GSLC specification XMLs * Revert changes to the GCOV and GSLC specification XMLs * round posting after conversion to decimeters commit 3f7027d Author: SamNemo <11642807+nemo794@users.noreply.github.com> Date: Thu Jun 18 10:29:43 2026 -0700 Fix text encoding: Replace en dash (U+2013) with ASCII hyphen in descriptions (#292) Replaced Unicode en dash character (U+2013 "–") with standard ASCII hyphen (U+002d "-") in 19 locations across 4 Python files. The en dash was causing display issues in some programs, appearing as "â€"" instead of "–". Files modified: - python/packages/isce3/signal/rfi_freq_null.py (1 occurrence in citation) - python/packages/nisar/products/insar/GUNW_writer.py (5 occurrences in mask descriptions) - python/packages/nisar/products/insar/GOFF_writer.py (4 occurrences in mask descriptions) - python/packages/nisar/products/insar/InSAR_L1_writer.py (9 occurrences in mask descriptions) This fix ensures HDF5 attribute descriptions display correctly across all programs and avoids confusion with special Unicode characters in source code. Co-authored-by: Samantha C. Niemoeller <samantha.c.niemoeller@jpl.nasa.gov> commit db4f65b Author: Gustavo H. X. Shiroma <52007211+gshiroma@users.noreply.github.com> Date: Wed Jun 17 14:59:11 2026 -0700 Add margins to the STATIC product Doppler LUT grid boundaries to ensure full radargrid coverage. (#298) * disable polarimetric symmetrization by default * revert changes to `symmetrize_cross_pol_channels` * Update GCOV and GSLC specification XMLs * Revert changes to the GCOV and GSLC specification XMLs * Add margins to the Doppler LUT grid boundaries to ensure full radargrid coverage. * Add margins to the Doppler LUT grid boundaries to ensure full radargrid coverage. * Add margins to the Doppler LUT grid boundaries to ensure full radargrid coverage. commit 67f0878 Author: Gustavo H. X. Shiroma <52007211+gshiroma@users.noreply.github.com> Date: Mon Jun 8 11:43:17 2026 -0700 Improve DEM bounding box selection when product coordinates are in a different projection than the DEM (#275) * disable polarimetric symmetrization by default * revert changes to `symmetrize_cross_pol_channels` * Update GCOV and GSLC specification XMLs * Revert changes to the GCOV and GSLC specification XMLs * Update DEM bounding box selection * Improve variable names, docstrings, and handle incorrect starting/ending ordering * Improve variable names * Ensure latitude values remain within [-90, 90] * Ensure latitude values remain within [-90, 90] * Simplify code * Use parenthesis to make it clear evaluation order * improve comments * revert changes to constructor of variables `all_x` and `all_y` * change margin types from `float` to `double`
5 participants
This PR is to fix the Ampcor issue