Skip to content

Comments

fix: fix starting lower bound when finding precise access AOS#648

Merged
alex-liang3 merged 2 commits intomainfrom
users/alexl/fix-access-crossing
Feb 16, 2026
Merged

fix: fix starting lower bound when finding precise access AOS#648
alex-liang3 merged 2 commits intomainfrom
users/alexl/fix-access-crossing

Conversation

@alex-liang3
Copy link
Contributor

@alex-liang3 alex-liang3 commented Feb 13, 2026

Fixes a bug introduced by #583 where, when finding the precise AOS of an access, an earlier access (not found by the coarse search) could be found, leading to the resulting access being more than 1 orbit long.

Summary by CodeRabbit

  • Bug Fixes

    • Improved precision of crossing calculations by refining interval boundary handling to avoid incorrect fallback behavior.
  • Tests

    • Added a new test validating precise crossing detection in edge-case orbital scenarios.

@alex-liang3 alex-liang3 self-assigned this Feb 13, 2026
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 13, 2026

No actionable comments were generated in the recent review. 🎉


📝 Walkthrough

Walkthrough

Reworks precise-crossing interval boundary handling: computes an interval-previous-step instant and sets the previous lower-bound as either that step (for the first interval) or the max of that step and the previous interval’s end plus one step. Adds tests validating coarse-search-missed accesses don't affect precise crossings.

Changes

Cohort / File(s) Summary
Precise Crossing Boundary Logic
src/OpenSpaceToolkit/Astrodynamics/Access/Generator.cpp
Introduces intervalPreviousStep and updates lowerBoundPreviousInstant computation to handle the first interval vs. subsequent intervals using max(...); updates comments. No public API changes.
Test Coverage for Boundary Handling
test/OpenSpaceToolkit/Astrodynamics/Access/Generator.test.cpp
Adjusts a comment in ComputeAccesses_7 and adds ComputeAccesses_8 (appears duplicated) to assert coarse-search misses are not used in precise-crossing computation.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • vishwa2710
  • apaletta3

Poem

🐰 I hopped through intervals, careful and spry,
I nudged the step where previous bounds lie,
First hop is gentle, following ones align,
Coarse misses no longer disrupt my sign,
Tests stamp the ground — precise times reply.

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically summarizes the main change: fixing a bug in the starting lower bound calculation when finding precise AOS (Acquisition of Signal) for access events.
Merge Conflict Detection ✅ Passed ✅ No merge conflicts detected when merging into main

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch users/alexl/fix-access-crossing

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@alex-liang3 alex-liang3 changed the title fix: fix precise crossing search start fix: fix starting lower bound when finding precise access crossings Feb 13, 2026
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@test/OpenSpaceToolkit/Astrodynamics/Access/Generator.test.cpp`:
- Around line 1247-1249: Remove the stray semicolon after the Velocity variable
declaration in Generator.test.cpp: the line ending with
"Velocity::MetersPerSecond({0.0, 8e3, 0.0}, Frame::ITRF()).inFrame(position,
Frame::GCRF(), orbitEpoch);" contains an extra ";" on the following line —
delete that extra semicolon so the declaration for velocity is clean (refer to
the Velocity::MetersPerSecond(...).inFrame(...) expression).

@alex-liang3 alex-liang3 changed the title fix: fix starting lower bound when finding precise access crossings fix: fix starting lower bound when finding precise access AOS Feb 13, 2026
@alex-liang3 alex-liang3 force-pushed the users/alexl/fix-access-crossing branch from 5cd2bf1 to 7752b49 Compare February 13, 2026 15:17
@codecov
Copy link

codecov bot commented Feb 13, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.95%. Comparing base (668f536) to head (d9dcdd9).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #648      +/-   ##
==========================================
+ Coverage   91.94%   91.95%   +0.01%     
==========================================
  Files         114      114              
  Lines       11118    11120       +2     
==========================================
+ Hits        10222    10225       +3     
+ Misses        896      895       -1     

☔ 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.

Copy link
Contributor

@apaletta3 apaletta3 left a comment

Choose a reason for hiding this comment

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

Great stuff! Left a few small comments for clarity

@alex-liang3 alex-liang3 enabled auto-merge (squash) February 16, 2026 14:37
@alex-liang3 alex-liang3 merged commit 2851844 into main Feb 16, 2026
22 checks passed
@alex-liang3 alex-liang3 deleted the users/alexl/fix-access-crossing branch February 16, 2026 15:05
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.

2 participants