[astro] Fix timezone regression in moon phase calculation

[Nadahar]

When the moon phase calculations were "modernized" in #20104, timezone was ignored when finding midnight, which led to inaccuracies that are dragged into further calculations.

This fixes the ignored timezone and takes that into account when finding "midnight". I can't quite explain how it can account for such a large error, but I haven't seen the exception logged since I corrected the midnight calculation. This can be because the earth and moon have moved while I figured it out, but it didn't take that long, so chances are that this actually fixes #20586.

[Copilot]

Pull request overview

Fixes Astro moon-phase inaccuracies caused by calculating midnight in UTC rather than the configured timezone.

Changes:

• Applies the timezone offset when deriving midnight.
• Uses the adjusted date for neighboring moon-phase calculations.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Nadahar]

Fair point.

[lsiepel]

Besides the corner case pointed out by copilot, i also like to add a non-blocking request to add unittests.
You could use the time and location somewhere close to your observations that lead to the discovery.

[Nadahar]

@lsiepel Sure, I guess there's no rush with this anyway. I'm in the middle of something else, so I just did this "on the side" and separated it out using stashing and some creative reverting.

We can always find back the moment I had the bug, because it's in the log in the issue. So, making a test for that time and some location around here should be fairly easy. I don't know if this fixes the issue, as I said, because even though the error had disappeared when I had made it, the window might simply have passed. But, this is clearly a bug regardless.

Making tests and more thorough fixing will have to wait until I'm done with the current "project", it's too much of a hassle to do this together with the other changes, and Eclipse had some serious issues getting things working again after my little "stash and switch" stunt.

[lsiepel]

Yeah, we should experience joy and no rush.

[wborn]

AI reviewed this PR before manual review.

I checked the change against the pre-#20104 implementation, the current MoonPhaseSet age calculation, the existing astro unit tests, the actual #20586 failure timing, and published moon-phase timestamps.

The timezone regression identified here is real, and this change appears to address the specific failure reported in #20586. However, there still appears to be a remaining regression from #20104: parentNewMoon, which is used to calculate the current lunar age, is now selected relative to midnight instead of the current time. If a new moon occurs during the local day, the calculated age can therefore remain based on the previous lunation until midnight.

The DST/midnight issue has already been covered by the existing Copilot review.

[wborn]

parentNewMoon still seems to be anchored to midnight here, which leaves another part of the #20104 regression in place.

Before #20104, midnight was used to find the remarkable phase dates for the local calendar day, while parentNewMoon was calculated separately from the current julianDate. That distinction seems important because lunar age is determined by the actual previous and next new moon instants, not by the start of the current calendar day.

The existing test provides a deterministic example: it has the Amsterdam new moon on 2019-03-06 at 17:04, which also agrees with published astronomical phase tables. If this calculation runs at 18:00 that day, julianDateMidnight is still 00:00. The backwards lookup therefore returns the February new moon while the forwards NEW lookup returns the already-past 17:04 new moon. getAgePercentDouble() consequently becomes greater than 1 instead of restarting close to zero after 17:04.

updateName() happens not to throw in this particular case because the entire local date is recognized as a NEW phase day, but the age percentage and degree are still outside their expected range.

Could the current lunation used for age be selected from julianDate again, while retaining the midnight-based lookup where it is needed to determine which remarkable phase belongs to the local calendar day? The requested regression tests could also cover immediately before and after a known new-moon timestamp.

[Nadahar]

@wborn Thanks for the analysis, I might as well try to fix that as well. But, could you please post these reviews as "comments" instead of "require change", because otherwise PRs will effectively be "blocked" until you make another approving review? In this case, for example, another preexisting bug is hardly a reason to block this PR.

Even though maintainers can choose to ignore the "require changes flag", I feel that the PR is more likely to die with such a review posted that is never "cleared". The "request changes flag" is general and applies to the whole PR; it doesn't get reset if you resolve the issues pointed out.

[wborn]

Thanks for the feedback. The AI review policy has been updated to distinguish more clearly between blocking and non-blocking findings.

"REQUEST_CHANGES" will now only be used when something genuinely needs to be addressed before the PR should be merged. Pre-existing issues, adjacent problems, optional improvements, and non-essential tests will normally be reported as non-blocking comments instead.

A pre-existing issue may still be blocking when the PR materially interacts with it—for example, when the proposed change would make it worse, leave the intended fix incomplete, or solidify an incorrect implementation.

This should also avoid leaving PRs unnecessarily blocked by a "CHANGES_REQUESTED" review after the actual review threads have been resolved.

[Nadahar]

Given that #20114 was just merged, I'm not sure that I want to do any more work on Astro - I certainly don't want to waste a second on fixing all the timezone issues that I had previously fixed and are being broken again because the code should be "modernized" when the existing code is/was working perfectly.

So, I think @clinique should fix the issues raised here as well, it's a "self-inflicted wound", and while I can't prevent them from happening, I don't see why I should "aid the process" by fixing them.

[lsiepel]

all the timezone issues that I had previously fixed and are being broken again

I might be missing something here, but I’m trying to understand what went wrong. The issues addressed by this PR didn’t seem to be affected by the changes in the other PR, so I’m not sure what was broken. At least from what I could see, there weren’t any conflicts.

When reviewing it, I paid close attention to the concerns you raised. Of course, it’s possible I overlooked something, but I didn’t expect this outcome. I’d appreciate some clarification on what specifically was problematic, so I can understand it and take it into account going forward.

[Nadahar]

Nothing "went wrong". I invested a lot of time in figuring out all the problems with time zones, update windows, events, etc. that existed in the binding, and as far as I know, everything ended up working correctly.

Then, all these PRs that refactor the working code into new code with an unknown "buggyness" started. That is true for any refactoring per se, but usually, you refactor because there is a problem with the current structure. That's not the case here, as far as I know at least. The refactoring is just about "modernization", whatever that means. To me, it mostly means that things become less efficient, since the more "modern" language features are usually those where performance is traded for convenience.

When you write new code, you might argue that "I'll use the "modern" features because that saves time when writing the code" (but usually not if you have to debug it). But, to "refactor" existing code that works into less efficient code just doesn't make any sense to me to begin with, so I'm generally not a fan of such refactorings. But, in this case, part of the "modernization" is to move from Calendar (which is timezone aware) to Instant (which isn't timezone aware). That makes the code fundamentally worse in my view, it's harder to do things correctly, and the end result might miss the essential timezone information, even if you get it right. If it was refactored to ZonedDateTime instead of Instant, I wouldn't be "against" it, I would just think it was pointless.

I've tried to explain this on several occasions, but the argument is that "time zone doesn't matter, because DateTimeType throws it away anyway, so why should we care about it?". Which doesn't make sense to me. Yes, it's a problem that DateTimeType has been broken, but why use that as a reason to break the rest of the system? Why throw away working code that uses time zones with "modernized" code that discards time zones? It just makes it so much more work to get time zones working again.

Given that this is my perspective, it's pointless for me to spend time/effort fixing bugs while the code is being degraded in parallel. It just doesn't make sense, which is why I'm saying that it shouldn't fall on me to fix these things. I can't prevent you from "destroying" the binding, but I can choose not to take part in the process.

Given that it seems like the time zone issue won't be fixed anyway, my suggestion is going nowhere, and nobody else has done anything about it, I'll probably have to stay with OH 4.2 for eternity. I'm tired of trying to explain the obvious; clearly, nobody agrees with my view that time zones matter.

I had been hoping that time zones would be fixed in core, but have always had a "backup plan" that I could always use a modified core where it works. But, if all the bindings should be broken as well, it becomes untenable. That means that the only viable option is to stay with 4.2, and that I should rather spend my time fixing problems there.

[lsiepel]

Thanks for explaining.

I interpreted your earlier response as meaning that the other PR had somehow broken the fixes in this PR, which is why I was confused. From your explanation, I understand now that this isn't really about something in this PR being technically wrong or conflicting with the other changes. Rather, you don't want to continue investing time in fixing the binding while it is being refactored in a direction you fundamentally disagree with, particularly regarding time zone handling. I can understand that, even if I don't necessarily agree with all of your conclusions about the direction being taken.

What I would like to avoid, though, is throwing away the work you already put into this fix. You mentioned yourself that you invested a lot of time understanding these issues, and simply closing the PR means that effort and the resulting fix are effectively lost as well.

If you're okay with it, I would therefore like to either reinstate the PR and take it from there, or use the code you've already written and raise a new PR myself. I don't expect you to continue maintaining or fixing it if you don't want to take part in this direction. I just don't want a useful fix, and the effort you already put into it, to be lost because of that.

[Nadahar]

The fix here is flawed as pointed out by Copilot, but it's easy to "bring along". What took time was to figure out what went wrong, not to fix it. Anybody is free to take this and further improve it, it's open source after all, but I won't spend the time fixing the other issue or making tests for it.