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

Filtering module - added reflect boundary fill #472

Closed
wants to merge 30 commits into from

Conversation

seyeonjeon
Copy link
Contributor

Added reflect boundary fill using np.pad in filtering module.

image

seyeonjeon and others added 28 commits June 4, 2024 17:17
Co-authored-by: Scott Staniewicz <[email protected]>
Co-authored-by: Scott Staniewicz <[email protected]>
Co-authored-by: Scott Staniewicz <[email protected]>
Co-authored-by: Scott Staniewicz <[email protected]>
Co-authored-by: Scott Staniewicz <[email protected]>
Co-authored-by: Scott Staniewicz <[email protected]>
Co-authored-by: Scott Staniewicz <[email protected]>
Co-authored-by: Scott Staniewicz <[email protected]>
Co-authored-by: Scott Staniewicz <[email protected]>
Co-authored-by: Scott Staniewicz <[email protected]>
Co-authored-by: Scott Staniewicz <[email protected]>
Co-authored-by: Scott Staniewicz <[email protected]>
Co-authored-by: Scott Staniewicz <[email protected]>
Co-authored-by: Scott Staniewicz <[email protected]>
Co-authored-by: Scott Staniewicz <[email protected]>
…o filtering

Conflicts:
	src/dolphin/filtering.py
	tests/test_filtering.py
…o filtering

Conflicts:
	src/dolphin/filtering.py
reflect fill using np.pad
Copy link
Collaborator

@scottstanie scottstanie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main concern besides the unit tests are failing is- what would happen on a frame with half-filled data? that is, for some coastal areas, the image could still be large, but mostly nodata; will that error? or just produce sub-optimally filled results (not as bad)

def _fill_boundary_area(unw_ifg_interp: np.ndarray, mask: np.ndarray) -> np.ndarray:
"""Fill the boundary area by reflecting pixel values."""
# Rotate the data to align the frame
rt_unw_ifg = ndimage.rotate(unw_ifg_interp, 8.6, reshape=False)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we'll want a parameter for the rotation in degrees:

  • inputs in radar coordinates will need no rotation
  • ascending and descending geocoded inputs would take the opposite rotation amounts

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we'll also probably want some parameter to indicate wither there's any fill requested at all- a radar image filled all the way to the borders doesn't need this reflection

Comment on lines +114 to +136
rt_pad = 700 # Apply padding for rotating back
# Fill boundary area using np.pad for each horizontal step and
reflect_filled = np.pad(
rt_unw_ifg[846:6059, :],
((846, rt_unw_ifg.shape[0] - 6059), (0, 0)),
mode="reflect",
)
reflect_filled = np.pad(
reflect_filled[618:6241, :],
((618, rt_unw_ifg.shape[0] - 6241), (0, 0)),
mode="reflect",
)
reflect_filled = np.pad(
reflect_filled[399:6447, :],
((399 + rt_pad, rt_unw_ifg.shape[0] - 6447 + rt_pad), (0, 0)),
mode="reflect",
)
# Fill vertical boundary area using np.pad
reflect_filled = np.pad(
reflect_filled[:, 591:8861],
((0, 0), (591 + rt_pad, rt_unw_ifg.shape[1] - 8861 + rt_pad)),
mode="reflect",
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for these numbers, we'll want to note the assumptions behind them

  • how they were found
  • what size images they apply to (i believe it's part of why the unit tests are failing)
  • ideally giving some kind of names, rather than leaving them as magic numbers

@scottstanie scottstanie requested a review from mirzaees December 4, 2024 02:51
@scottstanie
Copy link
Collaborator

Closing in favor of #520

@scottstanie scottstanie closed this Jan 7, 2025
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.

2 participants