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

DEM improvements to nodata, buffer and other corner cases #321

Merged
merged 9 commits into from
Mar 30, 2022

Conversation

chapulina
Copy link
Contributor

@chapulina chapulina commented Mar 15, 2022

🦟 Bug fix

Summary

Various small fixes to work with some DEMs I have:

Buffer to min height

Set buffer height to minimum height value. Non-square DEMs are automatically padded to become a square. The original logic is using height == 0 for the padding, which is very awkward for DEMs below ground. This PR changes the bufferVal to match the minimum height on the heightmap because a cliff down should be better than a cliff up, which is visible from far away.

Example:

Before (cliff up) After (cliff down)
portuguese_ledge pt_ledge

nodata comparisons

  • All comparisons to NaN return false, which caused the if (d > noDataValue) check to prevent calculating the min / max values. I fixed that by guarding explicitly for NaNs.
  • Another assumption in the current code is that the nodata value is smaller than all heights on the DEM. That's not the case for some DEMs I have here, which have very high values for nodata. So instead of checking if (d > noDataValue) I'm doing math::equal(d, noDataValue).

Dem::LoadData indentation

The entire function was too indented, so I fixed that as part of this PR. I recommend hiding whitespace changes to see what's actually changed.

GetProjectionRef and GetGeoTransform

These calls were returning values for my DEMs which weren't being well-handled. I added a guard and reduced an error to a debug message because the DEMs still work without these.

NetCDF

I also went ahead and used a NetCDF file (.nc) in a test to make sure we keep supporting this format.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Mar 15, 2022
@chapulina chapulina added the geospatial Geospatial component label Mar 15, 2022
@codecov
Copy link

codecov bot commented Mar 15, 2022

Codecov Report

Merging #321 (ddd8add) into main (c06cbb6) will increase coverage by 0.00%.
The diff coverage is 82.97%.

❗ Current head ddd8add differs from pull request most recent head 921af7c. Consider uploading reports for the commit 921af7c to get more accurate results

@@           Coverage Diff           @@
##             main     #321   +/-   ##
=======================================
  Coverage   77.12%   77.13%           
=======================================
  Files          76       76           
  Lines       10528    10540   +12     
=======================================
+ Hits         8120     8130   +10     
- Misses       2408     2410    +2     
Impacted Files Coverage Δ
geospatial/src/Dem.cc 90.99% <82.97%> (-0.47%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c06cbb6...921af7c. Read the comment docs.

Signed-off-by: Louise Poubel <[email protected]>
@chapulina chapulina added the MBARI-LRAUV Sponsored by MBARI-LRAUV project: https://github.com/osrf/lrauv label Mar 16, 2022
@chapulina chapulina changed the title Various DEM tweaks DEM improvements to nodata, buffer and other corner cases Mar 30, 2022
@chapulina chapulina marked this pull request as ready for review March 30, 2022 04:16
@chapulina chapulina requested a review from mjcarroll as a code owner March 30, 2022 04:16
Signed-off-by: Louise Poubel <[email protected]>
…-common into chapulina/dem

Signed-off-by: Louise Poubel <[email protected]>
Copy link
Contributor

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

One place for const, LGTM.

geospatial/src/Dem.cc Outdated Show resolved Hide resolved
Signed-off-by: Louise Poubel <[email protected]>

Co-authored-by: Michael Carroll <[email protected]>
@chapulina chapulina merged commit d266053 into main Mar 30, 2022
@chapulina chapulina deleted the chapulina/dem branch March 30, 2022 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden geospatial Geospatial component MBARI-LRAUV Sponsored by MBARI-LRAUV project: https://github.com/osrf/lrauv
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants