Skip to content

SPACalculator: return NaN when the rise/set iteration does not converge - #293

Closed
dickermoshe wants to merge 1 commit into
KosherJava:masterfrom
dickermoshe:spa-unconverged-secant
Closed

dickermoshe wants to merge 1 commit into
KosherJava:masterfrom
dickermoshe:spa-unconverged-secant

Conversation

@dickermoshe

@dickermoshe dickermoshe commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

The bug

solveRiseSet makes 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:

for (int i = 0; i < 12 && Math.abs(f1) > 1e-9; i++) { ... }
return t1;   // returned even when f1 is still far from 0

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: getUTCSunrise on 2127-03-14 at -77.2688, -92.8693 (elevation 3965 m), zenith 100.2 (Misheyakir 10.2°).

  • The Sun only gets about 10.04° below the horizon that night, so there is no 10.2° time.
  • The iteration goes 397 → 345 → 472 → 295 → -2857 → … → about -594,000 minutes.
  • Before this change, the method returns 10.8626 (04:51 local). At that time the Sun is about 8° away from the requested angle.

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:

Where the iteration ended Cases
below 1e-7° 1,825,998
1e-7° to 1e-6° (largest 8e-7°) 22,201
1e-6° to 1e-4° 18
1e-4° and above 2,684
  • Good results stop around 1e-7° to 8e-7°, not 1e-9°. That level is floating-point noise. jdDay + t1 / 1440.0 is 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.)
  • Failed runs end far away: 98.5% of them at 1e-3° or more, often tens of degrees off.
  • Why not something tighter like 1e-6°: it would sit right next to the good results (8e-7°), so a slightly different machine or JVM could throw away a real answer. 1e-4° is 125 times above the worst good result.
  • Why not something looser: 1e-4° is still smaller than the SPA's own stated accuracy (±0.0003°, Reda & Andreas). Anything accepted is at the target angle as closely as SPA itself can tell.

Tests

  • SPACalculatorTest.unconvergedRiseSetIsNoEvent covers the example above.
  • Without the fix it fails ("expected NaN but got 10.862627129964494"). With the fix, all 130 tests pass. Both runs were on the fork's CI.

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

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

1 participant