Repository navigation
SPACalculator: return NaN instead of running for hours when the target zenith is NaN - #299
Merged
Merged
Conversation
…get zenith is NaN A NaN or infinite zenith, or a NaN / infinite refraction, solar radius or a negative / NaN earth radius (which make the adjusted zenith NaN), made the bracketing rise / set search split every interval down to 1 ms because no interval of a NaN function can be discarded (about 2^27 SPA evaluations). NOAA and Meeus return NaN for the same input. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Er4Sx7LnKzzCbgDEatxkU
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 issue
SPACalculator.getUTCSunrise/getUTCSunsetnever return (in practice) when the zenith they search for isNaN. The other calculators returnNaNfor the same input in about 10 ms.The edge case
The target is the adjusted zenith, so it becomes
NaNwhenever any input to it isn't finite or makes no sense:getUTCSunrise(date, geo, Double.POSITIVE_INFINITY, false)calc.setRefraction(Double.NaN)calc.setSolarRadius(Double.POSITIVE_INFINITY)calc.setEarthRadius(-6371)These usually come from a bug or bad configuration in the calling app (a
0 / 0offset, an uninitialized setting), not from a deliberate request. They were found by fuzzing the library with random inputs.What happens
bracketRiseSetsearches a function of the formsin(elevation) - sin(90° - adjustedZenith), which isNaNeverywhere.CrossingSearch.finddiscards an interval only when a comparison proves it can't contain a crossing, and every comparison withNaNis false. So nothing is ever discarded, and the search splits every interval all the way down toTIME_TOLERANCE(1 ms over a 24 h window, about 2²⁷ SPA evaluations). The call doesn't finish in any reasonable time. In an app it freezes the thread; on a server it holds a worker indefinitely.The fix
getUTCSunRiseSet: If the adjusted zenith isn't finite, returnDouble.NaNright away, the same "no event" result the other calculators give. The calendar then returnsnullas usual.CrossingSearch.find: As defense in depth, returnNaNwhen either endpoint value isNaN, so any other source of aNaNfunction value (aNaNlatitude, for example) can't trigger the exhaustive search either.Finite zeniths never produce
NaNfunction values, so results for valid input are unchanged; the full test suite passes.Tests
SPACalculatorTest.nonFiniteZenithReturnsNaNPromptly(10 s timeout) covers a NaN and an infinite zenith, a negative earth radius, a NaN refraction and an infinite solar radius. Without the fix it times out.🤖 Generated with Claude Code
https://claude.ai/code/session_015Er4Sx7LnKzzCbgDEatxkU