-
Notifications
You must be signed in to change notification settings - Fork 38
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
Add endpoint for getting asset annotations #1113
base: main
Are you sure you want to change the base?
Add endpoint for getting asset annotations #1113
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing these endpoints. It looks as if the endpoint for getting sensor annotations was also actually returning the annotations on the sensor's asset rather than on the sensor itself.
Also, please check GH Actions, which is reporting a mypy issue on this PR. |
…for-getting-asset-annotations
Signed-off-by: Ahmad Wahid <[email protected]>
…ations Signed-off-by: Felix Claessen <[email protected]>
@@ -355,6 +359,8 @@ def search_annotations( | |||
annotation_type: str = None, | |||
include_account_annotations: bool = False, | |||
as_frame: bool = False, | |||
sensor_id: int = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be named asset_id
(or asset_or_sensor_id
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I don't think we need it. See my comment about using calling this method on the instance rather than the class.
""" | ||
event_starts_after = kwargs.get("event_starts_after", None) | ||
event_ends_before = kwargs.get("event_ends_before", None) | ||
if asset_or_sensor is GenericAsset: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is always False
, because this compares an Asset or Sensor instance (in asset_or_sensor
) to an Asset class.
|
||
|
||
def get_annotations_data( | ||
sensor_id: int | None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This parameter seems redundant, since we are already passing the sensor or asset instance itself.
if asset_or_sensor is GenericAsset: | ||
asset_or_sensor_class = asset_or_sensor.generic_asset | ||
else: | ||
asset_or_sensor_class = asset_or_sensor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if asset_or_sensor is GenericAsset: | |
asset_or_sensor_class = asset_or_sensor.generic_asset | |
else: | |
asset_or_sensor_class = asset_or_sensor | |
asset_or_sensor_class = asset_or_sensor.__class__ |
else: | ||
asset_or_sensor_class = asset_or_sensor | ||
|
||
df = asset_or_sensor_class.search_annotations( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need the class, actually? Isn't search_annotations
a method we can call on the instance rather than the class? Its function signature does suggest so, because its first parameter is self
rather than cls
.
df = asset_or_sensor_class.search_annotations( | |
df = asset_or_sensor.search_annotations( |
) | ||
from flexmeasures.data.models.data_sources import DataSource | ||
|
||
|
||
def query_asset_annotations( | ||
asset_id: int, | ||
asset_or_sensor_id: int, | ||
relationship_module, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing type annotation, possibly an Enum with the 3 relationship choices we currently have (with Accounts, GenericAssets and Sensors).
…for-getting-asset-annotations
In this PR, I have add an
AssetAPI
endpoint in thedev
module to fetch annotations data. Also I have refactored annotations query to be to get data for both asset and sensor annotations.