fix: correct axis order in DEM elevation sampling - #872
Conversation
add_elevation_from_dem() hardcoded a swap of the coordinate transform's output (mapX = result[1], mapY = result[0]), assuming the transform always returns (lat, lon)-order results. This only holds for geographic CRS rasters (like the app's default WGS84 DEM), where GDAL's default authority-compliant axis order is (lat, lon). For a projected CRS raster (e.g. UTM) - a realistic case for a user-uploaded custom DEM - authority-compliant order already matches (x, y)/(easting, northing), so the same hardcoded swap corrupts an otherwise-correct transform instead of fixing a swapped one. Points get sampled at the wrong pixel (or outside the raster entirely, silently falling back to elevation 0). Fix: explicitly set both SpatialReference objects (rasterSR, pointSR) to OAMS_TRADITIONAL_GIS_ORDER, which always resolves to (x, y) regardless of the underlying CRS's authority-declared order. This removes the need to guess/hardcode a swap at all, and is correct for both geographic and projected DEMs. Added a regression test (tests/test_add_elevation_from_dem.py) using a synthetic UTM-projected raster - the specific case that actually exercises the bug, verified to fail against the old code and pass against this fix. Fixes hotosm#718 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Brian Bergstrom <dulcetberg@gmail.com>
Contributor Signature RequiredThank you for your contribution! Before we can accept your pull request, you need to sign our Contribution Policy. Why do I need to do this?
How to signTo sign the agreement, please comment on this PR with: I have read the CONTRIBUTING.md document and I hereby sign and agree with the guidelines You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
for more information, see https://pre-commit.ci
|
Sorry I have to ask: are you an AI? Multiple PRs across many repos in such a short time. AI generated descriptions. You can never be sure 😅 |
Admittedly I am human but I did use Claude AI to help me with the code, clearly. I’m rather new to this and was exploring my options. I’m very sorry if this is against the rules but I figured it’d be a good way to contribute and learn. I’ve taken a few classes on geospatial python and understand the terminology well enough but I’m not good with code. The AI has helped me understand a bit and I’m still learning. Sorry for the confusion, |
|
Thanks Brian - appreciate the honesty! I'm very happy to welcome new contributors, but there are a few things to consider with AI / LLM usage: Perhaps more importantly, as a contributor: https://responsibleai.guide/ai-assisted-coding-guide/ Thanks for already disclosing the AI usage in the descriptions - it's really helpful 🙏 The key thing to consider here is that while generating the code may take your a few minutes, it can take the maintainer multiple hours to review and test each PR thoroughly - a huge time drain. I would propose:
We really appreciate you trying to help out with all these open issues! |
|
Thanks for the thoughtful reply — I appreciate you taking the time to lay that out rather than just shutting it down. I went and actually read through the responsible AI guide you linked, not just skimmed it. A few things stuck with me: the "you are the author, you are accountable" framing, and the point about testing changes yourself rather than just handing over something that runs. That second one especially I realized today that I'd been letting Claude run the verification and report back to me, instead of running it myself and seeing the results firsthand. That's on me to fix, not a small thing. But it was indeed me making all the critical decisions about the code once it was explained. Going forward: I'm going to slow down, read through everything line by line before it goes out, write my own PR descriptions, and actually test changes myself — I do geospatial Python in Jupyter as part of my background, so that's a natural place for me to verify things. Like you suggested, I'll start including screenshots of that testing on future PRs, so you can see I actually ran and checked the code myself rather than just taking my word for it. I'm new to open source and clearly still learning the norms here. I really appreciate you being direct with me about it instead of just closing the door. I'd like to keep contributing the right way, as I think this work is really cool. I'm unemployed and looking for volunteer GIS hours to count towards professional experience in my GISP application. A paying job is not what I am after; what I really need is the experience and feedback from people like you, as I have very little and GIS is my passion. I'm tired of applying to countless jobs and getting crickets, so I thought I would take the risk this way, even if it fails. Again, I appreciate your feedback and you not closing the door, and next time will be more thought out. Thank you for your time and look forward to making more contributions where I can. — Brian |
|
Thanks for understanding 😄 I'm very happy to work with you & help support any code changes you may wish to make. We work with many junior / new developers at HOT, so as long as the PRs are in good faith and in line with the responsible ai guide, then I'm more than happy to help 👍 |
Summary
Fixes #718.
add_elevation_from_dem()(indrone_flightplan/add_elevation_from_dem.py) hardcoded a swap of the coordinate transform's output:This assumes the transform always returns
(lat, lon)-order results. That only holds for geographic CRS rasters (like the app's default WGS84 DEM), where GDAL's default authority-compliant axis order happens to be(lat, lon).For a projected CRS raster (e.g. UTM) — a realistic case for a user-uploaded custom DEM, per the issue's own note about "different types of DEMs users may upload themselves" — authority-compliant order already matches
(x, y)/(easting, northing). The same hardcoded swap then corrupts an otherwise-correct transform instead of fixing a swapped one, so points get sampled at the wrong pixel (or land outside the raster entirely, silently falling back to elevation0).Fix
Explicitly set both
SpatialReferenceobjects (rasterSR,pointSR) toOAMS_TRADITIONAL_GIS_ORDERright after they're built, which always resolves to(x, y)regardless of what the underlying CRS's authority declares. This removes the need to guess/hardcode a swap at all — the fix is correct for both geographic and projected DEMs, not just the default case.Test plan
tests/test_add_elevation_from_dem.py(new — this package had no test coverage before). Builds a synthetic UTM-projected raster (the specific case that actually exercises the bug — a plain WGS84 raster does not, since the old swap happens to cancel out GDAL's implicit one in that case) and asserts the elevation sampled at a known point matches the known pixel value.AssertionError: Expected elevation 6.0 ..., got 0.0) and passes against the fix.🤖 Generated with Claude Code