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

Conversation

Flix6x
Copy link
Contributor

@Flix6x Flix6x commented Sep 12, 2022

This PR lets the asset page also show sensor data from other assets that belong to the same account.

…count

Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
@Flix6x Flix6x added the UI label Sep 12, 2022
@Flix6x Flix6x added this to the 0.12.0 milestone Sep 12, 2022
@Flix6x Flix6x self-assigned this Sep 12, 2022
@Flix6x Flix6x added this to In progress in Update UI views for Sensors and Assets via automation Sep 12, 2022
@coveralls
Copy link
Collaborator

coveralls commented Sep 12, 2022

Pull Request Test Coverage Report for Build 3219270038

  • 1 of 14 (7.14%) changed or added relevant lines in 3 files are covered.
  • 9 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.009%) to 64.999%

Changes Missing Coverage Covered Lines Changed/Added Lines %
flexmeasures/api/v3_0/sensors.py 0 1 0.0%
flexmeasures/data/models/generic_assets.py 0 1 0.0%
flexmeasures/data/services/sensors.py 1 12 8.33%
Files with Coverage Reduction New Missed Lines %
flexmeasures/data/services/sensors.py 1 17.86%
flexmeasures/utils/time_utils.py 8 64.26%
Totals Coverage Status
Change from base Build 3036791653: -0.009%
Covered Lines: 6479
Relevant Lines: 9359

💛 - Coveralls

Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

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

Great. Just one small signature simplification.

@@ -7,14 +7,25 @@


def get_sensors(
account_id: 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.

I believe we should keep this to one argument.

If it's int: query for account ID.
If it's string: query for name.
Otherwise, it's an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Your idea also gave me the idea to avoid some duplicate queries to the Account table, and to filter the query of the sensors table.

Update UI views for Sensors and Assets automation moved this from In progress to Reviewer approved Sep 12, 2022
…d or name), and avoid duplicate queries of a known account

Signed-off-by: F.N. Claessen <felix@seita.nl>
…ame owner

Signed-off-by: F.N. Claessen <felix@seita.nl>
… from the same account, which may be a lot)

Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
…ng_sensor_data_from_other_assets_in_the_same_account

# Conflicts:
#	documentation/changelog.rst
@@ -7,23 +7,32 @@


def get_sensors(
account_name: str | None = None,
account: Account | int | str | None = 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.

Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
…t, or None for public sensors (which makes get_public_sensors obsolete)

Signed-off-by: F.N. Claessen <felix@seita.nl>
@Flix6x
Copy link
Contributor Author

Flix6x commented Sep 12, 2022

Sorry to ask you to review once more. Your suggested name change led me to repurpose get_account_sensors() to require passing an Account, after which I saw an opportunity to simplify by merging it with the logic from get_public_sensors(). The resulting function can now be used to get sensors from multiply accounts (as well as public sensors) in a single query.

Update UI views for Sensors and Assets automation moved this from Reviewer approved to Review in progress Sep 12, 2022
Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

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

Getting closer :D

)
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.


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.

@@ -7,23 +7,32 @@


def get_sensors(
account_name: str | None = None,
account: Account | int | str | None = 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.

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

…gain, and rename filter argument

Signed-off-by: F.N. Claessen <felix@seita.nl>
Update UI views for Sensors and Assets automation moved this from Review in progress to Reviewer approved Sep 24, 2022
Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

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

I approve with two small conditions (sorry)

:param account_name: optionally, filter by account name.
:param accounts: optionally, select only sensors from this list of accounts
(include None to select sensors that belong to a public asset).
:param filter_by_sensor_ids: optionally, filter by sensor id.
Copy link
Contributor

Choose a reason for hiding this comment

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

"optionally, filter out any sensors which are not in this whitelist"


:param account_name: optionally, filter by account name.
:param accounts: optionally, select only sensors from this list of accounts
(include None to select sensors that belong to a public asset).
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a fan of the None argument to indicate public assets.
I believe an explicit parameter "include_public_assets", which defaults to False, would make it easier to see what's going on.

Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
@Flix6x Flix6x merged commit f6ad242 into main Oct 11, 2022
Update UI views for Sensors and Assets automation moved this from Reviewer approved to Done Oct 11, 2022
@Flix6x Flix6x deleted the issue-499_Allow_showing_sensor_data_from_other_assets_in_the_same_account branch October 11, 2022 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants