fix: sunrise/sunset times being identical when Phase 3 convergence ends below threshold - #241
Merged
rt-bishop merged 1 commit intoAug 19, 2026
Conversation
Owner
|
Hey there! Only managed to get to this today. The change makes sense, although ideally I'd prefer to see some unit tests added with those various locations you mentioned. That would protect the functionality against potential breaks in the future and also make this PR much more useful/complete. |
…gence ends below threshold The findSunRiseSet algorithm's Phase 3 converges on the sunrise elevation using a symmetric stepping approach. Due to the convergence step size, the final elevation can land slightly below the -0.8333° threshold (e.g. -0.838° instead of -0.8333°). When this happens, Phase 4's while-loop condition (sunPos.elevation > -threshold) evaluates to false immediately, causing it to skip entirely. Phase 5 then converges to the same point, producing identical sunrise and sunset times. The fix adds a small offset (daynum = sunrise + 0.001, ~1.4 minutes) at the start of Phase 4, ensuring the sun is clearly above the threshold before the sunset search begins. This is a deterministic fix — the bug was reproducible 100% of the time for any location where the convergence lands below threshold (e.g. equator at equinox). Fixes: sunrise/sunset showing identical times (~30% of location/date combinations)
atsunatsu
force-pushed
the
fix/sunrise-sunset-convergence
branch
from
August 19, 2026 12:41
5a23bcf to
9971f4c
Compare
Contributor
Author
|
Thanks! I added JVM unit tests covering equinox regression cases plus representative southern/northern hemisphere locations, and verified with |
rt-bishop
approved these changes
Aug 19, 2026
rt-bishop
left a comment
rt-bishop
left a comment
Owner
There was a problem hiding this comment.
Yay, that's much better! Thanks for doing it so quickly =)
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
2 participants
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.
Bug description
The
findSunRiseSetalgorithm sometimes returns identical sunrise and sunset times. This happens when Phase 3 converges to an elevation slightly below the -0.8333 deg threshold (e.g. -0.838 deg instead of -0.8333 deg). Phase 4'ssunPos.elevation > -thresholdcheck fails immediately, skipping the sunset search, and Phase 5 converges to the same point.Root cause
Phase 2 uses a coarse step (0.008 daynum approx 11.5 min) to find the approximate sunrise point, jumping from below-threshold to well above it. Phase 3 then converges by symmetric stepping, but the step size
0.004 * deltacauses the final elevation to land below threshold in about 30% of cases.Fix
Start Phase 4 from
daynum = sunrise + 0.001(~1.4 minutes after sunrise), ensuring the sun is clearly above the threshold before the sunset search begins. This is a deterministic fix - the bug was reproducible 100% of the time for locations like the equator at equinox.Reproduction
Call
CelestialComputer.findSunRiseSet(GeoPos(0.0, 0.0), any time on March 20 or September 23). The sunrise and sunset times will be identical.Verification
Tested with a Python port of the algorithm against 52 location/date combinations. The fix resolves all cases where sunrise/sunset were identical without affecting the remaining cases.