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

Mix in timely beliefs Sensor #13

Merged
merged 12 commits into from Feb 14, 2021
Merged

Conversation

Flix6x
Copy link
Contributor

@Flix6x Flix6x commented Feb 1, 2021

  • Mix in timely beliefs Sensor.
  • Add custom database revision.

Add custom database revision.
@Flix6x Flix6x added the Data label Feb 1, 2021
@Flix6x Flix6x added this to the Data model based on timely beliefs milestone Feb 1, 2021
@Flix6x Flix6x self-assigned this Feb 1, 2021
@Flix6x Flix6x linked an issue Feb 1, 2021 that may be closed by this pull request
@Flix6x
Copy link
Contributor Author

Flix6x commented Feb 2, 2021

Note that I also set the following defaults for sensors:

  • for physical sensors: ex-post (right after delivery)
  • for economic sensors: ex-ante (right before delivery)

Where do you think that information should live, documentation wise?

@Flix6x
Copy link
Contributor Author

Flix6x commented Feb 2, 2021

Two todos refer to the use of knowledge horizons/times:

todo: take into account knowledge horizon function
api/common/utils/validators.py
Refers to the "prior" API parameter for selecting beliefs formed prior to some datetime. Tackling this todo is an enhancement of its own, because it will make it possible to use the "prior" field to get data from economic sensors, for which the heuristic (knowledge_time = start + duration) for physical sensors usually doesn't apply. Note that we currently have no getPriceData endpoint.

todo: interpret belief horizons with respect to knowledge time rather than event end.
data/queries/utils.py
Refers to filtering data using a belief_time_window (beliefs formed within a certain time period). Only the API currently uses this (the UI doesn't) to support the "prior" field mentioned above. Tackling this todo is a prerequisite for the API enhancement mentioned above, but also opens up the possibility for time traveling in the UI.

Do you feel like I should tackle these issues within this PR? For me it feels like they deserve a PR of their own.

@Flix6x Flix6x requested a review from nhoening February 2, 2021 16:13
@nhoening
Copy link
Contributor

nhoening commented Feb 2, 2021

First question: Do our existing tests adequately cover functionality touched here?

@Flix6x
Copy link
Contributor Author

Flix6x commented Feb 3, 2021 via email

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.

Looks like a very nice PR to me.

I have a few questions, and I am responding to your question about design of func store identifiers.

flexmeasures/conftest.py Show resolved Hide resolved
flexmeasures/data/models/assets.py Outdated Show resolved Hide resolved
flexmeasures/conftest.py Show resolved Hide resolved
flexmeasures/conftest.py Show resolved Hide resolved
@nhoening
Copy link
Contributor

nhoening commented Feb 4, 2021 via email

@Flix6x
Copy link
Contributor Author

Flix6x commented Feb 8, 2021

More todo items linked to this issue:

Todo: until we sorted out the ex_post_horizon, use all available price data
data/services/forecasting.py
Refers to determining what data to mask when querying for auto-regressor data. At first glance it looks like the ex_post_horizon could straightforwardly be gotten from the sensor's knowledge_horizon_fnc, but this is only the case for power and weather sensors with the k_h_fnc=ex_post. For markets, we'd probably be better off switching to non-rolling forecasts and using a knowledge_time_window to mask auto-regressor data.

Todo: replace 0 hours with whatever the moment of switching from ex-ante to ex-post is for this generic asset
api/v1_1/implementations.py
Refers to a rule to decide under which circumstances POSTed data should result in forecasting jobs being created. With the introduction of knowledge horizon functions, there is actually no need anymore to replace 0 hours with something that depends on the sensor's knowledge_horizon_fnc, and the todo can simply go.

@Flix6x
Copy link
Contributor Author

Flix6x commented Feb 8, 2021

@nhoening I propose to limit the scope of this PR to the introduction of the TB mixin, and to deal with the paradigm shift of knowledge horizon functions in a separate PR, because this paradigm shift touches many parts of our code and I'd like to prevent unnecessarily large PRs. To ensure forecasting and scheduling functionality is not affected, we could choose to set the knowledge horizon function of day-ahead markets to 0h ex_post for the time being, which represents the previous state of affairs. Tariffs, power sensors and weather sensors would already be properly moved to the new state of affairs. The latter two will not actually "feel" the change, while the tariff data will become much simpler and can still be used for scheduling.

@nhoening
Copy link
Contributor

nhoening commented Feb 8, 2021

Alright, sounds good. I trust you with the tariff data here.

@Flix6x Flix6x changed the title Mix in timely beliefs Sensor. Mix in timely beliefs Sensor Feb 14, 2021
@Flix6x Flix6x merged commit 15a44cb into main Feb 14, 2021
@Flix6x Flix6x deleted the issue-12-Mix_in_Sensor_from_Timely_Beliefs branch February 14, 2021 14:57
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.

Mix in Sensor from Timely Beliefs
2 participants