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

Sensor view #99

Merged
merged 27 commits into from Apr 29, 2021
Merged

Sensor view #99

merged 27 commits into from Apr 29, 2021

Conversation

Flix6x
Copy link
Contributor

@Flix6x Flix6x commented Apr 12, 2021

This PR adds a new UI view for individual Sensors, initially meant to shows Altair graphs of sensor data. The ability to graph sensor data is implemented as a new Sensor method, which is accessible via the API. This PR currently exposes all sensor attributes to the API, but we may choose to limit that to only a list of specific attributes to expose.

For now I propose that API functions relating to Sensors are not part of the official documentation, until the new data model is properly integrated.

Review instructions

flexmeasures db upgrade 04f0e2d2924a
flexmeasures add beliefs "example_data.csv" --sensor-id 66 --horizon 0

example_data.csv contains, for example:

Date,Inflow (kuub)
2020-12-03 14:00,212
2020-12-03 14:10,215.6
2020-12-03 14:20,203.8
2020-12-03 23:50,209.8
2020-12-04 22:50,109.8
2020-12-04 23:00,309.8
2020-12-05 23:00,59.8

New endpoints:

Get interactive view on the sensor data

http://localhost:5000/sensors/66/

Get sensor attributes

Name: http://localhost:5000/sensors/66/name/
Timezone: http://localhost:5000/sensors/66/timezone/
Chart specification: http://localhost:5000/sensors/66/chart/
Chart data within some time range: http://localhost:5000/sensors/66/chart_data/?event_starts_after=2020-12-03T14:05:00+00:00&event_ends_before=2020-12-03T14:15:00+00:00

Get sensor chart as standalone html

All data: http://localhost:5000/sensors/66/chart/?include_data=true&as_html=true
Within some time range: http://localhost:5000/sensors/66/chart/?include_data=true&as_html=true&event_starts_after=2020-12-03T14:05:00+00:00&event_ends_before=2020-12-03T14:15:00+00:00

@Flix6x Flix6x self-assigned this Apr 12, 2021
@Flix6x Flix6x changed the base branch from main to cli-csv-upload April 12, 2021 07:29
@Flix6x Flix6x requested a review from nhoening April 12, 2021 09:52
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 stuff.

I believe further discussion about some bigger design decisions is still to finalise between us. I already give some initial opinions. Also, some smaller stuff.

Question: from your earlier work (which you drew from here), the ability to add data to a chart is still to be applied right? Any other bigger items which are still not in this PR?

flexmeasures/data/models/time_series.py Outdated Show resolved Hide resolved
flexmeasures/data/models/time_series.py Outdated Show resolved Hide resolved
flexmeasures/data/models/time_series.py Show resolved Hide resolved
flexmeasures/ui/charts/__init__.py Outdated Show resolved Hide resolved
flexmeasures/data/models/time_series.py Outdated Show resolved Hide resolved
flexmeasures/ui/views/sensors.py Show resolved Hide resolved
flexmeasures/ui/templates/views/sensors.html Outdated Show resolved Hide resolved
flexmeasures/data/models/time_series.py Outdated Show resolved Hide resolved
flexmeasures/ui/views/sensors.py Outdated Show resolved Hide resolved
flexmeasures/ui/templates/views/sensors.html Outdated Show resolved Hide resolved
@Flix6x
Copy link
Contributor Author

Flix6x commented Apr 14, 2021

Question: from your earlier work (which you drew from here), the ability to add data to a chart is still to be applied right? Any other bigger items which are still not in this PR?

Indeed, I left out some major components of my earlier work. Just three, actually:

  1. Intelligent chart updating by requesting only data that isn't already available
  2. Plotting process data
  3. Time range requests for a specific timezone other than the timezone of the requested sensor

@nhoening
Copy link
Contributor

Maybe make issues for porting them over? At least the first one.

Base automatically changed from cli-csv-upload to main April 16, 2021 14:36
@Flix6x
Copy link
Contributor Author

Flix6x commented Apr 19, 2021

I made a separate issue for intelligent chart updating as you suggested.

@Flix6x Flix6x marked this pull request as ready for review April 20, 2021 09:18
@Flix6x Flix6x requested a review from nhoening April 20, 2021 09:27
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 ask for moving some functions around.

Also, from our discussion : I believe JSON endpoints should already be hooked into the /api/2_0 path. That would make it easier to see the difference and it's future proof right away. I believe that is not too hard. I could try my hand on it if you agree.

flexmeasures/data/models/time_series.py Outdated Show resolved Hide resolved
flexmeasures/data/models/time_series.py Outdated Show resolved Hide resolved
flexmeasures/ui/static/js/flexmeasures.js Outdated Show resolved Hide resolved
@Flix6x
Copy link
Contributor Author

Flix6x commented Apr 21, 2021

I ask for moving some functions around.

Also, from our discussion : I believe JSON endpoints should already be hooked into the /api/2_0 path. That would make it easier to see the difference and it's future proof right away. I believe that is not too hard. I could try my hand on it if you agree.

I thought we expressly didn't want to move them there, and only note our intentions to do so later. Already moving them into our versioned user API brings potential user confusion (given project #3) and a premature maintenance burden. I'd like to write these as if they are already user API endpoints, but keep them as developer API endpoints until more mature.

@nhoening
Copy link
Contributor

Maybe that was indeed the outcome of our discussion.

But then I miss the clear documentation in SensorAPI that it is a temporary placeholder, that they are only about returning JSON and that these endpoints will move into the /api/v_X/ namespace.

In the call, we discussed the /api/dev namespace, as well. We could also simply use a different route base here, so we got a clearer picture for the time being (this is actual output from flexmeasures routes which I amended with "-api":

SensorAPI:get                              GET        /sensor-api/<int:id>/
SensorAPI:get_chart                    GET        /sensor-api/<id>/chart/
SensorAPI:get_chart_data           GET        /sensor-api/<id>/chart_data/
SensorUI:get                                GET        /sensors/<int:id>/
SensorUI:get_chart                      GET        /sensors/<id>/chart/

@Flix6x
Copy link
Contributor Author

Flix6x commented Apr 22, 2021

Nice suggestion. I now moved the SensorAPI FlaskView into the API package in 65122e4 (and d0ee431, which I forgot to amend). This was the first time we registered a FlaskView instead of a Blueprint there, so please check my work. To test:

  • http://127.0.0.1:5000/sensors/<id> should give you the HTML view of a sensor
  • http://127.0.0.1:5000/api/dev/sensor/<id> should give you the JSON view of a sensor

And the dev "version" of the our API remained undocumented after running make update-docs.

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.

The usage of Flask-Classful looks correct! I researched what the main advantage is over Flask.MethodView, and it seems the one thing to point out is the ability to create custom verbs, not just for REST methods.
From https://github.com/teracyhq/flask-classful/Readme:

"Can't I just use the base classes in flask.views?
Well, yes and no. While flask.views.MethodView does provide some of the functionality of flask_classful.FlaskView it doesn't quite complete the picture by supporting methods that aren't part of the typical CRUD operations for a given resource, or make it easy for me to override the route rules for particular view."

flexmeasures/ui/views/sensors.py Outdated Show resolved Hide resolved
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 believe I found a regression from earlier changes.

flexmeasures/data/models/time_series.py Outdated Show resolved Hide resolved
* Move code to data package

Co-authored-by: F.N. Claessen <felix@seita.nl>
@Flix6x Flix6x added this to the 0.4.0 milestone Apr 29, 2021
Co-authored-by: Nicolas Höning <iam@nicolashoening.de>
@Flix6x Flix6x merged commit d23bf71 into main Apr 29, 2021
@Flix6x Flix6x deleted the sensor-view branch April 29, 2021 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants