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.

Sign up for GitHub

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

Jump to bottom

Statistics component fix device_class for incremental source sensors #88096

Merged
merged 6 commits into from Feb 15, 2023
Merged

Statistics component fix device_class for incremental source sensors #88096

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1a53624
Return None device_class for incremental source sensors
ThomDietrich Feb 14, 2023
9b696e4
Ignore linting error
ThomDietrich Feb 14, 2023
ff8aa00
Fix ignore linting error
ThomDietrich Feb 14, 2023
04611ad
Fix ignore linting error
ThomDietrich Feb 14, 2023
a47cf68
Fix ignore linting error
ThomDietrich Feb 14, 2023
7abe544
Catch potential parsing error with enum
ThomDietrich Feb 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
59 changes: 40 additions & 19 deletions homeassistant/components/statistics/sensor.py
View file
Open in desktop
Expand Up @@ -13,7 +13,8 @@

from homeassistant.components.binary_sensor import DOMAIN as BINARY_SENSOR_DOMAIN
from homeassistant.components.recorder import get_instance, history
from homeassistant.components.sensor import (
from homeassistant.components.sensor import ( # pylint: disable=hass-deprecated-import
DEVICE_CLASS_STATE_CLASSES,
frenck marked this conversation as resolved.
Show resolved Hide resolved
PLATFORM_SCHEMA,
SensorDeviceClass,
SensorEntity,
Expand Down Expand Up @@ -47,6 +48,7 @@
from homeassistant.helpers.start import async_at_start
from homeassistant.helpers.typing import ConfigType, DiscoveryInfoType, StateType
from homeassistant.util import dt as dt_util
from homeassistant.util.enum import try_parse_enum

from . import DOMAIN, PLATFORMS

Expand Down Expand Up @@ -144,7 +146,7 @@
}

# Statistics which retain the unit of the source entity
STAT_NUMERIC_RETAIN_UNIT = {
STATS_NUMERIC_RETAIN_UNIT = {
STAT_AVERAGE_LINEAR,
STAT_AVERAGE_STEP,
STAT_AVERAGE_TIMELESS,
Expand All @@ -166,7 +168,7 @@
}

# Statistics which produce percentage ratio from binary_sensor source entity
STAT_BINARY_PERCENTAGE = {
STATS_BINARY_PERCENTAGE = {
STAT_AVERAGE_STEP,
STAT_AVERAGE_TIMELESS,
STAT_MEAN,
Expand Down Expand Up @@ -296,15 +298,9 @@ def __init__(
self.ages: deque[datetime] = deque(maxlen=self._samples_max_buffer_size)
self.attributes: dict[str, StateType] = {}

self._state_characteristic_fn: Callable[[], StateType | datetime]
if self.is_binary:
self._state_characteristic_fn = getattr(
self, f"_stat_binary_{self._state_characteristic}"
)
else:
self._state_characteristic_fn = getattr(
self, f"_stat_{self._state_characteristic}"
)
self._state_characteristic_fn: Callable[
[], StateType | datetime
] = self._callable_characteristic_fn(self._state_characteristic)

self._update_listener: CALLBACK_TYPE | None = None

Expand Down Expand Up @@ -368,11 +364,11 @@ def _add_state_to_queue(self, new_state: State) -> None:
def _derive_unit_of_measurement(self, new_state: State) -> str | None:
base_unit: str | None = new_state.attributes.get(ATTR_UNIT_OF_MEASUREMENT)
unit: str | None
if self.is_binary and self._state_characteristic in STAT_BINARY_PERCENTAGE:
if self.is_binary and self._state_characteristic in STATS_BINARY_PERCENTAGE:
unit = PERCENTAGE
elif not base_unit:
unit = None
elif self._state_characteristic in STAT_NUMERIC_RETAIN_UNIT:
elif self._state_characteristic in STATS_NUMERIC_RETAIN_UNIT:
unit = base_unit
elif self._state_characteristic in STATS_NOT_A_NUMBER:
unit = None
Expand All @@ -393,11 +389,24 @@ def _derive_unit_of_measurement(self, new_state: State) -> str | None:
@property
def device_class(self) -> SensorDeviceClass | None:
"""Return the class of this device."""
if self._state_characteristic in STAT_NUMERIC_RETAIN_UNIT:
_state = self.hass.states.get(self._source_entity_id)
return None if _state is None else _state.attributes.get(ATTR_DEVICE_CLASS)
if self._state_characteristic in STATS_DATETIME:
return SensorDeviceClass.TIMESTAMP
if self._state_characteristic in STATS_NUMERIC_RETAIN_UNIT:
source_state = self.hass.states.get(self._source_entity_id)
if source_state is None:
ThomDietrich marked this conversation as resolved.
Show resolved Hide resolved
return None
source_device_class = source_state.attributes.get(ATTR_DEVICE_CLASS)
if source_device_class is None:
ThomDietrich marked this conversation as resolved.
Show resolved Hide resolved
return None
sensor_device_class = try_parse_enum(SensorDeviceClass, source_device_class)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not need to parse the enum to try to look it up in DEVICE_CLASS_STATE_CLASSES.

if sensor_device_class is None:
Comment on lines +401 to +402
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wallrus could have been used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could certainly, but then black will line-break this logic into oblivion!? I already had arguably better code locally.
You wanna suggest something? Maybe you have a better idea than me

Copy link
Member

@frenck frenck Feb 15, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My suggestion was already above, just wallrus it. We don't worry about formatting, that is what black is for.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't that contradict the wish for easily readable and maintainable code?

I'll have a look

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that is what black is for. You dont' have to agree with its style, but it is the style we are using consistently. The formatting is maintained by black and thus not a concern.

return None
Comment on lines +402 to +403
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add coverage for this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do! This wasn't raised by pytest before the changes regarding DEVICE_CLASS_STATE_CLASSES

sensor_state_classes = DEVICE_CLASS_STATE_CLASSES.get(
sensor_device_class, set()
)
Comment on lines +404 to +406
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The creation of an empty set() here is not needed.

if SensorStateClass.MEASUREMENT not in sensor_state_classes:
Comment on lines +404 to +407
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wallrus could have been used.

return None
return sensor_device_class
return None

@property
Expand Down Expand Up @@ -472,8 +481,8 @@ async def async_update(self) -> None:
if timestamp := self._next_to_purge_timestamp():
_LOGGER.debug("%s: scheduling update at %s", self.entity_id, timestamp)
if self._update_listener:
self._update_listener()
self._update_listener = None
self._update_listener() # pragma: no cover
self._update_listener = None # pragma: no cover
Comment on lines +484 to +485
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When does this happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't exactly tell you. I did not develop this part of the code. Pytest always showed missing coverage for these two lines, hence the comment. Maybe the code can be removed or changed, but I didn't feel the need.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There shouldn't be any pragma: no cover in homeassistant code.
It is better to have reduced coverage than invalid coverage.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #88173

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not agree and don't see how it would make sense in this particular case. Anyhow, understand the incentive generally and won't fight with you.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the code is there, there must be a reason...

  • If it is dead code that can't be reached, then it should be removed
  • If it is not dead code, then we need to be reminded that coverage should be improved

The only time when no cover is in use is when we raise an exception for unreachable code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is not reason to introduce fake coverage. Epenet is right here, it should not have had these flags. I've therefore merged the PR to remove them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought @epenet I agree with you. I did not develop this code nor did I ever touch it. I believe it's "dead code" as you called it and will do some tests and provide a fix in a future PR.


@callback
def _scheduled_update(now: datetime) -> None:
Expand Down Expand Up @@ -563,6 +572,18 @@ def _update_value(self) -> None:
value = int(value)
self._value = value

def _callable_characteristic_fn(
self, characteristic: str
) -> Callable[[], StateType | datetime]:
"""Return the function callable of one characteristic function."""
function: Callable[[], StateType | datetime] = getattr(
self,
f"_stat_binary_{characteristic}"
if self.is_binary
else f"_stat_{characteristic}",
Comment on lines +582 to +583
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't allow the use of multi-line inline if-statements; as they become hard to read. Please adjust this in a followup PR to have a regular if statement.

)
return function

# Statistics for numeric sensor

def _stat_average_linear(self) -> StateType:
Expand Down
120 changes: 112 additions & 8 deletions tests/components/statistics/test_sensor.py
View file
Open in desktop
Expand Up @@ -21,6 +21,7 @@
SERVICE_RELOAD,
STATE_UNAVAILABLE,
STATE_UNKNOWN,
UnitOfEnergy,
UnitOfTemperature,
)
from homeassistant.core import HomeAssistant
Expand Down Expand Up @@ -250,6 +251,63 @@ async def test_sensor_source_with_force_update(hass: HomeAssistant):
assert state_force.attributes.get("buffer_usage_ratio") == round(9 / 20, 2)


async def test_sampling_boundaries_given(hass: HomeAssistant):
"""Test if either sampling_size or max_age are given."""
assert await async_setup_component(
hass,
"sensor",
{
"sensor": [
{
"platform": "statistics",
"name": "test_boundaries_none",
"entity_id": "sensor.test_monitored",
"state_characteristic": "mean",
},
{
"platform": "statistics",
"name": "test_boundaries_size",
"entity_id": "sensor.test_monitored",
"state_characteristic": "mean",
"sampling_size": 20,
},
{
"platform": "statistics",
"name": "test_boundaries_age",
"entity_id": "sensor.test_monitored",
"state_characteristic": "mean",
"max_age": {"minutes": 4},
},
{
"platform": "statistics",
"name": "test_boundaries_both",
"entity_id": "sensor.test_monitored",
"state_characteristic": "mean",
"sampling_size": 20,
"max_age": {"minutes": 4},
},
]
},
)
await hass.async_block_till_done()

hass.states.async_set(
"sensor.test_monitored",
str(VALUES_NUMERIC[0]),
{ATTR_UNIT_OF_MEASUREMENT: UnitOfTemperature.CELSIUS},
)
await hass.async_block_till_done()

state = hass.states.get("sensor.test_boundaries_none")
assert state is None
state = hass.states.get("sensor.test_boundaries_size")
assert state is not None
state = hass.states.get("sensor.test_boundaries_age")
assert state is not None
state = hass.states.get("sensor.test_boundaries_both")
assert state is not None


async def test_sampling_size_reduced(hass: HomeAssistant):
"""Test limited buffer size."""
assert await async_setup_component(
Expand Down Expand Up @@ -514,9 +572,9 @@ async def test_device_class(hass: HomeAssistant):
{
"sensor": [
{
# Device class is carried over from source sensor for characteristics with same unit
# Device class is carried over from source sensor for characteristics which retain unit
"platform": "statistics",
"name": "test_source_class",
"name": "test_retain_unit",
"entity_id": "sensor.test_monitored",
"state_characteristic": "mean",
"sampling_size": 20,
Expand All @@ -537,6 +595,14 @@ async def test_device_class(hass: HomeAssistant):
"state_characteristic": "datetime_oldest",
"sampling_size": 20,
},
{
# Device class is set to None for any source sensor with TOTAL state class
"platform": "statistics",
"name": "test_source_class_total",
"entity_id": "sensor.test_monitored_total",
"state_characteristic": "mean",
"sampling_size": 20,
},
]
},
)
Expand All @@ -549,11 +615,21 @@ async def test_device_class(hass: HomeAssistant):
{
ATTR_UNIT_OF_MEASUREMENT: UnitOfTemperature.CELSIUS,
ATTR_DEVICE_CLASS: SensorDeviceClass.TEMPERATURE,
ATTR_STATE_CLASS: SensorStateClass.MEASUREMENT,
},
)
hass.states.async_set(
"sensor.test_monitored_total",
str(value),
{
ATTR_UNIT_OF_MEASUREMENT: UnitOfEnergy.WATT_HOUR,
ATTR_DEVICE_CLASS: SensorDeviceClass.ENERGY,
ATTR_STATE_CLASS: SensorStateClass.TOTAL,
},
)
await hass.async_block_till_done()

state = hass.states.get("sensor.test_source_class")
state = hass.states.get("sensor.test_retain_unit")
assert state is not None
assert state.attributes.get(ATTR_DEVICE_CLASS) == SensorDeviceClass.TEMPERATURE
state = hass.states.get("sensor.test_none")
Expand All @@ -562,6 +638,9 @@ async def test_device_class(hass: HomeAssistant):
state = hass.states.get("sensor.test_timestamp")
assert state is not None
assert state.attributes.get(ATTR_DEVICE_CLASS) == SensorDeviceClass.TIMESTAMP
state = hass.states.get("sensor.test_source_class_total")
assert state is not None
assert state.attributes.get(ATTR_DEVICE_CLASS) is None


async def test_state_class(hass: HomeAssistant):
Expand All @@ -572,17 +651,28 @@ async def test_state_class(hass: HomeAssistant):
{
"sensor": [
{
# State class is None for datetime characteristics
"platform": "statistics",
"name": "test_nan",
"entity_id": "sensor.test_monitored",
"state_characteristic": "datetime_oldest",
"sampling_size": 20,
},
{
# State class is MEASUREMENT for all other characteristics
"platform": "statistics",
"name": "test_normal",
"entity_id": "sensor.test_monitored",
"state_characteristic": "count",
"sampling_size": 20,
},
{
# State class is MEASUREMENT, even when the source sensor
# is of state class TOTAL
"platform": "statistics",
"name": "test_nan",
"entity_id": "sensor.test_monitored",
"state_characteristic": "datetime_oldest",
"name": "test_total",
"entity_id": "sensor.test_monitored_total",
"state_characteristic": "count",
"sampling_size": 20,
},
]
Expand All @@ -596,14 +686,28 @@ async def test_state_class(hass: HomeAssistant):
str(value),
{ATTR_UNIT_OF_MEASUREMENT: UnitOfTemperature.CELSIUS},
)
hass.states.async_set(
"sensor.test_monitored_total",
str(value),
{
ATTR_UNIT_OF_MEASUREMENT: UnitOfTemperature.CELSIUS,
ATTR_STATE_CLASS: SensorStateClass.TOTAL,
},
)
await hass.async_block_till_done()

state = hass.states.get("sensor.test_nan")
assert state is not None
assert state.attributes.get(ATTR_STATE_CLASS) is None
state = hass.states.get("sensor.test_normal")
assert state is not None
assert state.attributes.get(ATTR_STATE_CLASS) is SensorStateClass.MEASUREMENT
state = hass.states.get("sensor.test_nan")
state = hass.states.get("sensor.test_monitored_total")
assert state is not None
assert state.attributes.get(ATTR_STATE_CLASS) is None
assert state.attributes.get(ATTR_STATE_CLASS) is SensorStateClass.TOTAL
state = hass.states.get("sensor.test_total")
assert state is not None
assert state.attributes.get(ATTR_STATE_CLASS) is SensorStateClass.MEASUREMENT


async def test_unitless_source_sensor(hass: HomeAssistant):
Expand Down