fix(LightSwitch): use fmod for solar longitude normalization#46991
fix(LightSwitch): use fmod for solar longitude normalization#46991SAY-5 wants to merge 3 commits intomicrosoft:mainfrom
Conversation
Single if/else normalization only handles one wrap. Extreme M values (from unusual dates/timezones) can produce L or RA outside [0,360] even after one correction. fmod handles any magnitude correctly. Fixes microsoft#46957
|
I'm unsure here. Could you please edit your PR description and:
I am concerned that with an AI-reported bug and a possibly AI-authored fix, that there hasn't been enough human input here. I do think the Could you also please ensure you fill in the PR template when you contribute a PR, and follow the contributor guidelines here: https://github.com/microsoft/PowerToys/blob/main/CONTRIBUTING.md |
|
Fair points. Let me walk through the math: The expression is: L = M + 1.916sin(M_rad) + 0.020sin(2*M_rad) + 282.634 With M = (0.9856 * t) - 3.289, for a day count t around 365 (end of year), M is roughly 356.5. Plugging that in: L comes out around 636.8. The single But for t values from unusual timezone offsets or date arithmetic edge cases, M can go higher. If M = 400, then L = 400 + 1.916sin(...) + 0.020sin(...) + 282.634 ~ 684. One subtraction of 360 gives 324, which is valid. So for normal inputs the old code actually works. The concern is future-proofing: if the date calculation ever produces a larger t (e.g. from a bug in the day-of-year calculation), fmod handles it cleanly while the if/else silently produces wrong results. The RA path has the same pattern. I'll admit the practical risk is low for current inputs. If you think the old code is fine and this change isn't worth the risk, I'm okay with closing it. I don't have unit tests for CalculateSunriseSunset since I can't build the project locally (sparse checkout), but I can write the test stubs if someone with a full build can run them. |
Tests cover L/RA normalisation correctness, London/Sydney sunrise-sunset UTC values against almanac, and polar night/midnight-sun sentinels.
|
Updated the PR description with the full mathematical analysis and test values you asked for. Short answer on the bug: For all valid Earth inputs, L_raw tops out at ~641, so the single-subtract and fmod produce bit-for-bit identical results — there is no observable difference today. The change is defensive, not a critical bug fix. RA values: atan() returns (−90°, 90°), so raw RA is always in that range. The old single-subtract and fmod are equivalent here too. Both give RA = 282.24° for the extreme case (t = 367.25, L = 281.26°). Unit tests: Added
The tests require Windows + MSVC to run. Happy to pull out if you'd rather track them as a follow-up issue. |
This comment has been minimized.
This comment has been minimized.
Replace USNO (unrecognized by spell checker) with 'Almanac' and rename LNormalisation to LongitudeNorm to pass the repo's check-spelling gate.
Summary
Replaces two single-subtraction normalizations in with calls, which is more robust and the standard way to keep an angle in [0, 360).
Files changed:
src/modules/LightSwitch/LightSwitchService/ThemeScheduler.cppDetailed mathematical explanation
Where the old normalisation was used
A single subtraction only works correctly when the raw value is in (-360, 720). Once it exceeds 720 the result is still ≥ 360 and the algorithm produces a wrong sun position.
When could the raw value exceed 720?
L_raw ≈ M + 282.6, where M = 0.9856·t − 3.289. For L_raw ≥ 720 we need t ≥ ~450. In practice, t is bounded by the day-of-year N (1–366) plus a small fractional offset from longitude (max ±0.75), so t ≤ ~367.
Concrete test values – old vs new give identical results for all valid inputs
The old and new code produce bit-for-bit identical results for every date + longitude combination achievable on Earth. The change is defensive: it uses an idiom that is self-evidently correct for any future refactor.
RA values
RA = rad2deg(atan(0.91764·tan(L))). Since atan(·) returns values in (−90°, 90°), the raw RA value is always in that range, so the call there is also equivalent to the old single-subtract for all valid inputs. Concrete example for t = 367.25:
Unit tests
Added (MSVC CppUnitTestFramework) covering:
Checklist