Daily high/low temperature calculation issue

cloneofghosts commented 1 month ago

Describe the bug

I've noticed that the daily high and low temperatures do not take into account 6pm for the daily high and 6am for the daily low. If the daily high temperature is at 6pm then that temperature isn't shown as the daily high. Same goes for the nightly low if its at 6am then it also isn't used as the daily low temperature.

Expected behavior

Daily high should include 6pm and daily low should include 6am when calculating the high and low temperatures

Actual behavior

Daily high does not include 6pm and daily low does not include 6am when calculating the high and low temperatures

API Endpoint

Production

Location

Ottawa, Ontario

Other details

I had posted this comment in #239 a few weeks ago but it looks like it was missed so posting here for better visibility.

Troubleshooting steps

[X] I have searched this repository and Home Assistant Repository to see if the issue has already been reported.
[X] I have read through the API documentation before opening this issue.
[X] I have written an informative title.

alexander0042 commented 1 month ago

Another good catch! So for example, the daily high should go from 6:01 to 18:00, but it's currently going from 6:00 to 17:59? It's a great question, and honestly one that I'd never really thought about. I think the most intuitive times would be:

Daily high: 6:01 to 18:00 (inclusive)
Overnight low: 18:01 to 6:00 (inclusive)

This is a super easy fix, so I'll roll it into the update this week

cloneofghosts commented 1 month ago

I thought the daily high/low temperatures would be calculated based on the hour and not based on the minute considering the models only have 1h time-steps.

I think the end time for the daily high/low temperatures just need to be extended by 1h and it should be good. This could lead to a situation where one days low has the same time as the next days high but it seems pretty unlikely.

alexander0042 commented 1 month ago

You're correct, they are based on the hour- I just used minutes there to be clear which hour was included or not, but might have made it more complicated. Solution makes sense, and I'll put it in.

cloneofghosts commented 1 month ago

I noticed that this issue seems to be fixed on 2.0.11 but when I check development version 2.1 it doesn't have the fix. Maybe it was just added to the wrong place?

alexander0042 commented 3 weeks ago

Ok, just migrated this fix over to the dev backend, so it'll be live in 2.1!

Pirate-Weather / pirateweather