Fix 0/0 = NaN in the rise/set parabola fit - #1140
Merged
brandon-rhodes merged 1 commit intoAug 4, 2026
Merged
Conversation
When the Newton iteration in _find() happens to land, to the last bit, exactly on the horizon, the final parabola refinement divided 0/0 and returned NaN as the event time. Fixes skyfielders#1114.
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.
Fixes #1114. The full analysis is in my comment on that issue; the short version:
By the time
_find()reaches its final parabola refinement, the Newton iteration has sometimes already converged so tightly that the last two sample times are numerically adjacent instants, and the recomputed altitude offset from the horizon is bit-identical at both. When that offset happens to land on exactly0.0,_q()divides0/0: the numerator-2*cis-0.0becausec = y0 = 0.0, and withy0 == y1the sign is+1, so the denominatorb + sqrt(b*b)is0.0wheneverb < 0. The resulting NaN survivesnp.clip()and is returned as the event time, whileis_above_horizonstaysTrue— NaN comparisons are False — so callers cannot filter the bad event and crash later inutc_iso()/utc_datetime()withValueError: cannot convert float NaN to integer.When
cis zero,x = 0is exactly a root of the quadratic — the curve passes through zero at the first sample point, which is where the iteration converged — so_q()now returns it directly. Theerrstateguard keeps NumPy quiet while the discarded quotient is computed.The trigger is last-ULP rounding of the whole trig pipeline, which is why the failing (date, latitude, body) tuples in the issue thread differ across platforms and NumPy builds. The new test therefore skips the ephemeris entirely and feeds
_intersection()the exact operands captured from a failing case (a Mars setting at 52.0°N on 2029-11-04, on Linux/aarch64 with NumPy 2.4.1): it computes NaN before this change and0.0after.With the fix, both deterministic cases from the thread return times that agree with an independent
find_discretesearch overrisings_and_settingson the same day:Test suite: the failing-test set under
assay --batch skyfield.testsis identical before and after the change (the failures on my machine are pre-existing NumPy 2.5 deprecation warnings escalated byPYTHONWARNINGS=error, unrelated to this fix), and the new test passes.pyflakesis clean.