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

Features/v3 annual averaging #352

Merged
merged 11 commits into from
Aug 13, 2024
Merged

Conversation

caparker
Copy link
Collaborator

Pull request fixes issues for annual averaging but we also discussed updated endpoints as well, instead of using the period_name.

For the path updates I suggest the following
sensors/1/measurements -- raw data for sensor 1
sensors/1/hours -- hourly data for sensor 1
sensors/1/days-- daily data for sensor 1
sensors/1/years -- annual data for sensor 1
sensors/1/measurements/hourly -- raw data aggregated to hourly for sensor 1 (really an alias for hourly data)
sensors/1/measurements/daily -- raw data aggregated to daily for sensor 1, we dont store this so this would be novel
sensors/1/days/yearly -- daily data for sensor 1 aggregated to a year
The nice thing about above is that it would turn the period_name into another resource. The only thing I worry about for something like this is that the combination of the base/aggregation could be confusing but it would have its own endpoint and therefor we could offer a great description.
Lets say that a user wants to see the annual average of daily values, it would be
sensors/1/days/yearly
which reads better to me than
sensors/1/measurements/daily?period_name=year
sensors/1/daily?period_name=year

  Also added the ability to change the base table from hourly to
  daily, though this has not been implimented yet.
@caparker caparker requested a review from russbiggs May 29, 2024 22:32
@russbiggs
Copy link
Member

A couple things I would like to work through and better understand:

This pattern feels a bit odd to me sensors/1/measurements/hourly this feel like it would more naturally be sensors/1/measurements?period=hourly, which is what we are moving away from obviously. I guess the double nested resources just looks foreign to me, I expect an id between, which obviously wouldnt make sense here.

I assume when you say "sensors/1/measurements/hourly is an alias hourly" for I am guessing you mean its functionally the is the same as sensors/1/hours. For hourly data this doesn't present as much of an issue but when looking at something like sensors/1/measurements/daily, is this also the same as sensors/1/days? (I assume not, based on our discussion I would guess the latter is based on hourly whereas the former will be the lowest granularity available for that sensor). To me this raises a question of what is the period used for each:

  • sensors/1/hours
  • sensors/1/days
  • sensors/1/years

Then the question is do we need other options beyond our chosen defaults? i.e. do we need the options for both daily-annual average and monthly-annual average (and "raw" annual average etc.). The other question would be can we realistically deliver every granularity in a performant enough way? Currently hour of day summaries for a single year is finnicky in terms of being fast enough for the timeout.

@caparker
Copy link
Collaborator Author

We are trying to address two questions,

  • what base table does the user want to pull from and then
  • how do they want to aggregate the data.

For the first, what table do they want to pull from I think it should look like this

sensors/1/measurements -- give me the raw measurements for sensor 1
sensors/1/hours -- give me the hourly aggregated data
sensors/1/days -- give me the daily aggregated data
sensors/1/years -- give me the annually aggregated data

The benefit of this is that we can have a different response model and filters for each resource.

For the second, how does the user want to aggregate the data, we have a few options, for example, lets say the user wants to take the raw measurements and aggregate them to daily values, we have the following options

sensors/1/measurements?aggregate=daily <--- treat it as a flag, same response shape
sensors/1/measurements/daily <--- treat it as a resource, different response shape

My assumption would be that the response from this call would be shaped differently than the response from the sensors/1/measurements call and therefor it makes the most sense use the path method.

But if the response was not different I could see going the aggregate=daily route
Here are the different combinations
measurements > hourly
measurements > daily
measurements > annually
hours > daily
hours > annually
days > annually
years > ?

We also have the trends aggregates to think about
hour of day - measurements, hours
day of week - measurements, hours, days
month of year - measurements, hours, days

@caparker
Copy link
Collaborator Author

caparker commented Jul 2, 2024

There is still a little to do, like clean up queries and responses but I wanted to get this out for review just to make sure this is the right direction. You can get a feel for the new paths by looking at the tests.

@russbiggs
Copy link
Member

The "raw" measurements endpoint /v3/sensors/{sensrs_id}/measurements for this is currently not working. Throwing this error:

ValueError: column tz.timezones_id does not exist

@caparker caparker changed the base branch from main to v3-release July 24, 2024 12:55
@russbiggs russbiggs marked this pull request as ready for review August 13, 2024 15:09
@russbiggs russbiggs merged commit 5898bb3 into v3-release Aug 13, 2024
russbiggs added a commit that referenced this pull request Aug 20, 2024
* add instruments_id query param for locations endpoint (#357)

* add instruments_id query param for locations endpoint

* Fixed typos in test

---------

Co-authored-by: Christian Parker <[email protected]>

* Features/v3 manufacturers query param (#358)

* add manufacturers_id parameter

* add manufacturers_id parameter to locations route

---------

Co-authored-by: Christian Parker <[email protected]>

* Some misc fixes for v3

* V3 licenses resources (#356)

* added licenses resources

* license resources

* Added validation and some tests, removed trailing spaces

* Added license router to main app

  - Moved the utils tests to the unit tests
  - remove locations from the url list because its covered in the
    sensors test now

* fixes missing columns for licenses

* license resource updates

* Just some spacing my editor fixed

---------

Co-authored-by: Christian Parker <[email protected]>

* Feature/rate limit headers redux (#363)


* update to rate limit algorithm and headers

* Feature/v3 providers updates (#364)


* remove licenses from providers and change owner_entity to entitiesId for v3/providers

* Features/v3 annual averaging (#352)

* Updated to let users aggregate to annual average

  Also added the ability to change the base table from hourly to
  daily, though this has not been implimented yet.

* Added ability to change base table to sensors

* Added new paths and tests and have basic working examples

* add display name for measurand

* revert

* Testing out a method where we can alter query strings
  This allows us to use the same query class even when we just need to
  alter the query field

* Updated tests and queries

  Not passing all tests yet but very close

* One test still failing: days -> moy

  The same query seems to work locally but not in the test?

* Fixed timestamp issues - passing all tests

---------

Co-authored-by: Russ Biggs <[email protected]>

* V3 locations fix (#365)

* add license query params and utils fix

* add licenses param tests

* Added the missing test fix

---------

Co-authored-by: Christian Parker <[email protected]>

---------

Co-authored-by: Christian Parker <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants