Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure sun conditions are using the right date #23664

Merged
merged 6 commits into from Aug 16, 2019

Conversation

@emontnemery
Copy link
Contributor

commented May 4, 2019

Description:

When evaluating sun conditions, take care to ensure we are comparing current time with solar event times for the correct day.

The added test cases:

  • test_if_action_before_sunrise_no_offset_kotzebue
  • test_if_action_after_sunrise_no_offset_kotzebue
  • test_if_action_before_sunset_no_offset_kotzebue
  • test_if_action_after_sunset_no_offset_kotzebue

all fail without this patch because events from the wrong (already passed) day were compared with.
The added test cases test sun condition at summer in Kotzebue, which has an extremely skewed local time zone: https://en.wikipedia.org/wiki/Time_zone#Skewing_of_zones, with the sun setting at 3AM local time and rising at 7 AM local time during the summer as illustrated in this chart.

Fix by testing if we got solar events from the previous local day, and if we do, use events for the next local day.

However, the sun conditions are still unintuitive with this change, after_sunset is for example true from 3 AM until midnight, whereas after_sunrise is true from 7 AM until midnight at summer in Kotzebue.

This means this condition will not work:

condition:
    condition: or  # 'when dark' condition: either after sunset or before sunrise - equivalent to a state condition on `sun.sun` of `below_horizon`
    conditions:
      - condition: sun
        after: sunset
      - condition: sun
        before: sunrise

This is very much related to the solar events using midnight in local time as a cutoff point instead of solar midnight (https://www.home-assistant.io/docs/scripts/conditions/#sun-condition).

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

@emontnemery emontnemery requested a review from armills May 4, 2019

@emontnemery emontnemery requested a review from home-assistant/core as a code owner May 4, 2019

@armills

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

Please open an architecture issue that explains these changes as requested in Discord.

@emontnemery

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

@armills What was discussed on discord was to change the definition of the sun conditions to, for example, use astronomical midnight as cutoff point. I'll open an architecture issue for that.
This PR is just an attempt to improve to make sure we at least use the right date when evaluating the conditions, but without changing the definition.

Edit: Architecture issue suggesting changing cutoff point for the sun conditions: home-assistant/architecture#225

@pvizeli

This comment has been minimized.

Copy link
Member

commented Jul 4, 2019

What is the state here?

@MartinHjelmare MartinHjelmare added this to Needs review in Dev Jul 23, 2019

@project-bot project-bot bot moved this from Needs review to Second opinion wanted in Dev Aug 9, 2019

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

What should we do here?

@balloob

This comment has been minimized.

Copy link
Member

commented Aug 10, 2019

I don't know enough about this topic to feel like I can make a clear decision.

My gut feeling tells me we should mark it as "after_sunrise" from when the sun rises till when it sets (so equivalent to when sun state is above_horizon). Similar for after_sunset.

@emontnemery

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

This PR intends to correct a bug where the wrong day is used when evaluating sun conditions. It would be great if the changes could be reviewed.

The discussion about if the sun conditions make sense or not are not related to the bug as such.
I proposed to change the cutoff point to astronomical midnight here home-assistant/architecture#225 to make the change non breaking for most cases and make it work for locations with very skewed time zones or within the polar circles.
Doing what @balloob is much more intuitive but would be a breaking change.

(Also, there is a warning in the docs against using the sun conditions: https://www.home-assistant.io/docs/scripts/conditions/#sunsetsunrise-condition )

sunset = sunset_today
if (today > dt_util.as_local(cast(datetime, sunrise_today)).date() and
SUN_EVENT_SUNRISE in (before, after)):
sunrise = sunrise_tomorrow

This comment has been minimized.

Copy link
@balloob

balloob Aug 12, 2019

Member

Let's only calculate tomorrow if we need it.


if (today > dt_util.as_local(cast(datetime, sunset_today)).date() and
SUN_EVENT_SUNSET in (before, after)):
sunset = sunset_tomorrow

This comment has been minimized.

Copy link
@balloob
@@ -279,6 +292,30 @@ def sun(hass: HomeAssistant, before: Optional[str] = None,
# There is no sunset today
return False

_LOGGER.warning("---------------------------------")

This comment has been minimized.

Copy link
@balloob

balloob Aug 12, 2019

Member

Stale logging ?

Dev automation moved this from Second opinion wanted to Reviewer approved Aug 12, 2019

@balloob
Copy link
Member

left a comment

This change looks good. Few small tweaks needed, ok to merge afterwards.

emontnemery added 5 commits May 1, 2019

@emontnemery emontnemery force-pushed the emontnemery:sun_conditions_alaska branch from c2a6dc4 to df4cf6a Aug 16, 2019

@emontnemery emontnemery changed the title RFC: Ensure sun conditions are using the right date Ensure sun conditions are using the right date Aug 16, 2019

@emontnemery

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

@balloob Comments fixed, thanks for the review!

@emontnemery emontnemery merged commit fad54cd into home-assistant:dev Aug 16, 2019

9 checks passed

CI Build #20190816.31 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA

Dev automation moved this from Reviewer approved to Done Aug 16, 2019

@emontnemery emontnemery deleted the emontnemery:sun_conditions_alaska branch Sep 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
6 participants
You can’t perform that action at this time.