Skip to content

Minor bug fix within terrain correction#121

Open
BStoneLASP wants to merge 2 commits into
mainfrom
bs_terrcorr_bugfix
Open

Minor bug fix within terrain correction#121
BStoneLASP wants to merge 2 commits into
mainfrom
bs_terrcorr_bugfix

Conversation

@BStoneLASP
Copy link
Copy Markdown
Collaborator

Minor bug fix within terrain correction that caused a DEM query error under an extreme pointing scenario (intersect ellipsoid with near 90deg off nadir pointing). Required for Clarreo end-to-end testing.

This same logic (excluding high off nadir points from DEM query) occurs later within the terrain correction. The bug was that it wasn't being excluded from the initial query. We never encountered an issue before because usually (every time before) the first query is within the bounding box of the queried DEM data.

Minor bug fix within terrain correction that caused a DEM query error under an extreme pointing scenario (intersect ellipsoid with near 90deg off nadir pointing).
@BStoneLASP BStoneLASP self-assigned this Mar 10, 2026
@tmorland77
Copy link
Copy Markdown
Collaborator

Looks good!

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 10, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.62%. Comparing base (996cc37) to head (6ded003).

❌ Your project check has failed because the head coverage (81.30%) is below the target coverage (95.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #121   +/-   ##
=======================================
  Coverage   73.62%   73.62%           
=======================================
  Files          67       67           
  Lines       10655    10656    +1     
  Branches     1204     1204           
=======================================
+ Hits         7845     7846    +1     
  Misses       2343     2343           
  Partials      467      467           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

gd_cur_alt[idx_cont] = gd_cur_pos_subset[:, 2]

# Look up the ellipsoidal height (surface) for this position.
gd_prev_h = gd_cur_h
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Only question is if you want to add this separate change to this PR:

gd_prev_h = gd_cur_h.copy()

Copy link
Copy Markdown
Collaborator

@mmaclay mmaclay left a comment

Choose a reason for hiding this comment

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

Looks good to go. There some subtle semantics changes as we now expect gd_cur_h will have NaNs from the beginning, but assuming the testing looks good, this should be an improvement overall.

I added a comment about changing line 941 only if you want to include it in this PR. I think we probably want to reference a copy not the original variable there, but feel free to leave for a later discussion.

@mmaclay
Copy link
Copy Markdown
Collaborator

mmaclay commented Apr 10, 2026

Just updated this branch w/ main to keep this PR from timeout closing automatically.

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.

3 participants