Skip to content

Correct right ascension to hour angle conversion - #149

Open
sylvesterkaczmarek wants to merge 1 commit into
llnl:mainfrom
sylvesterkaczmarek:fix/hour-angle-conversion
Open

sylvesterkaczmarek wants to merge 1 commit into
llnl:mainfrom
sylvesterkaczmarek:fix/hour-angle-conversion

Conversation

@sylvesterkaczmarek

Copy link
Copy Markdown
Contributor

Summary

rightascension_to_hourangle() currently mixes HMS and DMS conversions. Numeric local times are converted with dd_to_dms(), and the final angular difference is also formatted as DMS even though the public API promises an HMS hour angle. The documented examples therefore return the wrong value, midnight wrapping is incorrect, and equatorial_to_horizontal() receives a string when using right ascension plus local time with its default options.

Convert both inputs to decimal degrees, form the wrapped local_time - right_ascension angle, and format the result with dd_to_hms(). Convert that returned HMS value back to degrees inside equatorial_to_horizontal() before the trigonometric calculation.

Validation

Python 3.12 on macOS arm64:

  • New regressions on unchanged main (45730dc): 3 failed. After the fix: all 3 passed.
  • Focused utility coverage: 33 passed across the regression module and test_utils.py.
  • Full suite: 337 passed, 2 skipped, 35 subtests passed.
  • py_compile and git diff --check passed.
  • The new regression module passes Flake8; utils.py retains only its pre-existing Flake8 findings.

Regression coverage includes both documented input forms, wrap-through-midnight behavior, and the downstream equatorial_to_horizontal() path.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.39%. Comparing base (45730dc) to head (71eebab).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #149      +/-   ##
==========================================
- Coverage   97.39%   97.39%   -0.00%     
==========================================
  Files          17       17              
  Lines        5897     5893       -4     
==========================================
- Hits         5743     5739       -4     
  Misses        154      154              
Flag Coverage Δ
unittests 97.39% <100.00%> (-<0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
ssapy/utils.py 99.74% <100.00%> (-<0.01%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sylvesterkaczmarek
sylvesterkaczmarek marked this pull request as ready for review September 20, 2026 02:48
@sylvesterkaczmarek

Copy link
Copy Markdown
Contributor Author

This has been open for about 11 days with no maintainer feedback. The branch is current, mergeable, and all current checks are green. Could a maintainer review it when convenient?

This branch has not been deployed

No deployments
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