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 erroneous masks for Coreg classes #197

Merged
merged 3 commits into from
Jul 30, 2021

Conversation

erikmannerfelt
Copy link
Contributor

Fixes #195 .

Changes:

  • warp_dem() now has mask dilation by default.
  • get_array_and_mask() takes and properly respects Rasters now (it was used with rasters before, but the mask was not respected).
  • -9999 values in warp_dem() were replaced by np.nan
  • Added a warning catcher in apply_matrix() (due to skimage + numpy deprecation warning)
  • Improved test to validate that the above changes fixes the problem.

Something that is not tested automatically but was tested manually was the example by @adehecq in #195 in examples/plot_blockwise_coreg.py. As seen in the screenshot in #195, these fixes solve that problem.

- warp_dem() now has mask dilation by default.
- get_array_and_mask() takes and properly respects Rasters now (it was
  used with rasters before, but the mask was not respected).
- -9999 values in warp_dem were replaced by np.nan
- Added a warning catcher in apply_matrix() (due to skimage + numpy)
- Improved test to validate that the above changes fixes the problem.
@erikmannerfelt erikmannerfelt added the bug Something isn't working label Jul 30, 2021
@erikmannerfelt erikmannerfelt requested a review from adehecq July 30, 2021 09:21
@erikmannerfelt erikmannerfelt self-assigned this Jul 30, 2021
@erikmannerfelt
Copy link
Contributor Author

erikmannerfelt commented Jul 30, 2021

@adehecq, I've honestly forgotten why these lines need to exist, but they do right now:

x_inds = rio.fill.fillnodata(transformed_points[:, :, 0].copy(), mask=(~nan_mask).astype("uint8"))

y_inds = rio.fill.fillnodata(transformed_points[:, :, 1].copy(), mask=(~nan_mask).astype("uint8"))

If I replace them with your suggestion (just a copy of transformed_points), the test_nuth_kaab() fails since an edge effect becomes predominant.


# Validate that the zshift is not something crazy high and that no negative values exist in the data.
assert np.nanmax(np.abs(zshift)) < 50
assert np.count_nonzero(aligned.data.filled(0) < -50) == 0
Copy link
Member

Choose a reason for hiding this comment

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

I've seen you use 'filled' quite often. I believe your intention is to exclude nodata from the statistics. If so, I recently found out you can use data.compressed(), which returns only the valid values as a 1D array (I know, the name is not very intuitive...).

Copy link
Member

Choose a reason for hiding this comment

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

A better test would be to check that the elevation range before/after coregistration is more or less identical.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to use compressed() in 2d13111!

I also added tests to check that coregistration improved. I see now that I misread your suggestion haha, but this should be more than apt, no?

Copy link
Member

Choose a reason for hiding this comment

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

Well, the overall statistics might not be too sensitive to just a few outliers. But the range will be, so I would still recommend checking the range.
But it's a good idea to check that the dispersion is reduced too!

xdem/coreg.py Outdated Show resolved Hide resolved
adehecq
adehecq previously approved these changes Jul 30, 2021
Copy link
Member

@adehecq adehecq left a comment

Choose a reason for hiding this comment

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

I don't understand why those changes are needed, but if that fixes the issue and does not break any previous test, I guess it should be better!

@adehecq
Copy link
Member

adehecq commented Jul 30, 2021

Thanks for fixing the bugs!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blockwise coregistration apply method does not take nodata properly into account
2 participants