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 get_terrain_attribute bug with edge_method='none' #245

Merged
merged 15 commits into from
Nov 22, 2021

Conversation

rhugonnet
Copy link
Contributor

Resolves #244

  • Fixes an indexation issueof get_terrain_attribute with edge_method='none',
  • Removes wrong referencing of slope equation in comments,
  • Changes the default edge behaviour to match that of GDAL (i.e. edge_method='none' and fill_method=none`),
  • Adds doctests for slope convenience function, and adapt doctests of other attributes with new default methods.

We should have more specific, in-depth tests for the edge and filling methods. Not the aim of this PR, so opening an issue for it.

@rhugonnet rhugonnet changed the title Fix get_terrain_attribute edge method Fix get_terrain_attribute edge method Nov 21, 2021
@rhugonnet rhugonnet changed the title Fix get_terrain_attribute edge method Fix get_terrain_attribute bug with edge_method='none' Nov 21, 2021
Copy link
Contributor

@erikmannerfelt erikmannerfelt left a comment

Choose a reason for hiding this comment

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

Thanks for improving on these methods!

It's uncontroversial to default to the GDAL functionality, but I think GDAL is wrong in propagating NaNs like that. If you think we should default to GDAL-behavior, then sure.

xdem/terrain.py Show resolved Hide resolved
@rhugonnet
Copy link
Contributor Author

Thanks for improving on these methods!

It's uncontroversial to default to the GDAL functionality, but I think GDAL is wrong in propagating NaNs like that. If you think we should default to GDAL-behavior, then sure.

When you are missing only one point in the corner, I agree the results is still OK! But when you are missing half of the cell, the terrain attribute value will be aberrant.
Maybe an argument could control when the filling method is applied, depending on the amount of gaps to fill in the 3x3 window? Then I would certainly be fine having a default method that fills when there's only one missing pixel for example 😉
With the current approach, we should definitely default to GDAL to avoid all those pixels that end up with erroneous estimates (usually we have enough pixels in DEM data anyway!)

Copy link
Contributor

@erikmannerfelt erikmannerfelt left a comment

Choose a reason for hiding this comment

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

Great!

@rhugonnet rhugonnet merged commit d38b83b into GlacioHack:main Nov 22, 2021
@rhugonnet rhugonnet deleted the fix_terrainedge branch November 22, 2021 20:43
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.

Argument edge_method='none' of get_terrain_attribute returns void/constant grid
2 participants