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

Issue 499 allow showing sensor data from other assets in the same account #500

Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
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
1 change: 1 addition & 0 deletions documentation/changelog.rst
Expand Up @@ -9,6 +9,7 @@ New features
-------------

* Hit the replay button to replay what happened, available on the sensor and asset pages [see `PR #463 <http://www.github.com/FlexMeasures/flexmeasures/pull/463>`_]
* The asset page also allows to show sensor data from other assets that belong to the same account [see `PR #500 <http://www.github.com/FlexMeasures/flexmeasures/pull/500>`_]
* Improved import of time series data from CSV file: 1) drop duplicate records with warning, and 2) allow configuring which column contains explicit recording times for each data point (use case: import forecasts) [see `PR #501 <http://www.github.com/FlexMeasures/flexmeasures/pull/501>`_]

Bugfixes
Expand Down
4 changes: 2 additions & 2 deletions flexmeasures/api/v3_0/sensors.py
Expand Up @@ -41,7 +41,7 @@
from flexmeasures.data.schemas.sensors import SensorSchema, SensorIdField
from flexmeasures.data.schemas.units import QuantityField
from flexmeasures.data.schemas import AwareDateTimeField
from flexmeasures.data.services.sensors import get_sensors
from flexmeasures.data.services.sensors import get_account_sensors
from flexmeasures.data.services.scheduling import create_scheduling_job
from flexmeasures.utils.time_utils import duration_isoformat
from flexmeasures.utils.unit_utils import ur
Expand Down Expand Up @@ -110,7 +110,7 @@ def index(self, account: Account):
:status 403: INVALID_SENDER
:status 422: UNPROCESSABLE_ENTITY
"""
sensors = get_sensors(account_name=account.name)
sensors = get_account_sensors(accounts=[account])
return sensors_schema.dump(sensors), 200

@route("/data", methods=["POST"])
Expand Down
14 changes: 11 additions & 3 deletions flexmeasures/data/models/generic_assets.py
Expand Up @@ -431,18 +431,26 @@ def search_beliefs(
def sensors_to_show(self) -> List["Sensor"]: # noqa F821
"""Sensors to show, as defined by the sensors_to_show attribute.

Sensors to show are defined as a list of sensor ids, which
is set by the "sensors_to_show" field of the asset's "attributes" column.
Valid sensors either belong to the asset itself, to other assets in the same account,
or to public assets.


Defaults to two of the asset's sensors.
"""
if not self.has_attribute("sensors_to_show"):
return self.sensors[:2]

from flexmeasures.data.services.sensors import get_public_sensors
from flexmeasures.data.services.sensors import get_account_sensors

sensor_ids = self.get_attribute("sensors_to_show")
sensor_map = {
sensor.id: sensor
for sensor in self.sensors + get_public_sensors(sensor_ids)
if sensor.id in sensor_ids
for sensor in get_account_sensors(
accounts=[self.owner, None], # include public sensors
sensor_ids=sensor_ids,
)
}

# Return sensors in the order given by the sensors_to_show attribute
Expand Down
47 changes: 19 additions & 28 deletions flexmeasures/data/services/sensors.py
@@ -1,43 +1,34 @@
from __future__ import annotations

from werkzeug.exceptions import NotFound
import sqlalchemy as sa

from flexmeasures import Sensor, Account
from flexmeasures.data.models.generic_assets import GenericAsset


def get_sensors(
account_name: str | None = None,
def get_account_sensors(
Copy link
Contributor

Choose a reason for hiding this comment

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

If we also get sensors that are not part of any account, the word "account" is misleading again :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. :)

I can rename back to get_sensors and make accounts optional again. That would also make it more suitable for later expansion, as needed, with other filters, like limiting the search to a subset of assets.

accounts: list[Account | None],
sensor_ids: list[int] | None = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a good name imho. How about "sensor_id_whitelist" ? "sensor_id_filter"?'

I tend to think we don't need it.
We now have double logic as we already do if sensor.id in sensor_ids in sensors_to_show()

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 removed the double logic by removing the now redundant if statement in sensors_to_show().

Copy link
Contributor

Choose a reason for hiding this comment

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

I still think the name is not clear. It's used as a whitelist.

) -> list[Sensor]:
"""Return a list of Sensor objects.

:param account_name: optionally, filter by account name.
"""
sensor_query = Sensor.query

if account_name is not None:
account = Account.query.filter(Account.name == account_name).one_or_none()
if not account:
raise NotFound(f"There is no account named {account_name}!")
sensor_query = (
sensor_query.join(GenericAsset)
.filter(Sensor.generic_asset_id == GenericAsset.id)
.filter(GenericAsset.owner == account)
)

return sensor_query.all()


def get_public_sensors(sensor_ids: list[int] | None = None) -> list[Sensor]:
"""Return a list of Sensor objects that belong to a public asset.
"""Return a list of Sensor objects that belong to any of the given accounts.

:param accounts: select only sensors from this list of accounts
(include None to select sensors that belong to a public asset).
:param sensor_ids: optionally, filter by sensor id.
"""
sensor_query = (
Sensor.query.join(GenericAsset)
.filter(Sensor.generic_asset_id == GenericAsset.id)
.filter(GenericAsset.account_id.is_(None))
account_ids = [account.id for account in accounts if account is not None]
sensor_query = Sensor.query.join(GenericAsset).filter(
Sensor.generic_asset_id == GenericAsset.id
)
if None in accounts:
Copy link
Contributor

Choose a reason for hiding this comment

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

Confusing that one might pass [AccountA, AccountB, None] and only get back public sensors.

Simpler (no support for mixing): do not pass accounts at all to get public sensors. None cannot be part of the list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Confusing that one might pass [AccountA, AccountB, None] and only get back public sensors.

In this case you should get back public sensors as well as sensors from both accounts. Is the documentation not clear, or is there a problem with the code?

Simpler (no support for mixing): do not pass accounts at all to get public sensors. None cannot be part of the list.

That wouldn't yield the same benefit, because sensors_to_show() would need to call the function twice, resulting in two queries instead of one.

Copy link
Contributor

Choose a reason for hiding this comment

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

I overlooked line 26, sorry.

sensor_query = sensor_query.filter(
sa.or_(
GenericAsset.account_id.in_(account_ids),
GenericAsset.account_id.is_(None),
)
)
else:
sensor_query = sensor_query.filter(GenericAsset.account_id.in_(account_ids))
if sensor_ids:
sensor_query = sensor_query.filter(Sensor.id.in_(sensor_ids))
return sensor_query.all()