Repository navigation
SPACalculator: return NaN when the rise/set iteration does not converge - #293
Closed
dickermoshe wants to merge 1 commit into
Closed
dickermoshe wants to merge 1 commit into
dickermoshe wants to merge 1 commit into
Conversation
Near the poles the closed-form first guess can report an event the topocentric Sun never reaches. The secant iteration then diverges and solveRiseSet returned its last guess, an arbitrary time. Return NaN when the final zenith residual is above 1e-4 degrees. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Oct 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
solveRiseSetmakes a first guess, then improves it with up to 12 secant steps. If those steps never reach the target zenith, it returns the last guess anyway:Near the poles this happens on days when the Sun almost, but not quite, reaches the requested angle. The closed-form first guess, which uses the declination at noon, is slightly optimistic and says the event happens. The secant then searches for a time that doesn't exist, jumps further and further away, and its last guess is returned as if it were a zman.
Example:
getUTCSunriseon 2127-03-14 at -77.2688, -92.8693 (elevation 3965 m), zenith 100.2 (Misheyakir 10.2°).We found this by running randomized comparisons against kosher-rust, a Rust port. Both sides returned unconverged values, and they disagreed by 78 minutes because tiny floating-point differences sent the bad guesses in different directions.
The fix
After the loop, check how far the Sun's zenith at the returned time is from the target. If it's more than
MAX_ZENITH_RESIDUAL(1e-4°), the iteration did not converge, so return NaN: no event that day, the same as the existing polar day/night case. Converged results are unchanged, bit for bit.Why 1e-4°
We ran the same secant on about 3 million random cases (all latitudes, dates 1900–2300, zeniths from 89° to 119°, elevations 0–4000 m) and looked at how far from the target each run ended:
jdDay + t1 / 1440.0is a number around 2.4 million, so a double can only step its time by about 40 microseconds. In 40 µs the Sun's zenith moves up to about 1.7e-7°, so the loop can't get closer than that. (This also means the 1e-9 loop guard is almost never met, and the loop nearly always runs all 12 iterations.)Tests
SPACalculatorTest.unconvergedRiseSetIsNoEventcovers the example above.Not addressed here
Very close to the poles (|latitude| > 83°), the iteration sometimes converges properly, but onto a crossing on a neighbouring day. A residual check can't detect that, so it's left for a separate change.
🤖 Generated with Claude Code