-
Notifications
You must be signed in to change notification settings - Fork 38
Document & refactor scheduling specs for storage flexibility model #511
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
Conversation
Pull Request Test Coverage Report for Build 3494526674
💛 - Coveralls |
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.
The schedule's Source could use more work, but I'm really happy with where this is going.
…factor its parameters and handling within the code for readability Signed-off-by: Nicolas Höning <nicolas@seita.nl>
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
…rging_sooner part of storage specs & an optional parameter in API v3 Signed-off-by: Nicolas Höning <nicolas@seita.nl>
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
3255c89
to
c0400ae
Compare
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
…we use name, but also in the new setting, unless we always include the user_id) Signed-off-by: Nicolas Höning <nicolas@seita.nl>
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
One thing to consider: We might add a data migration, so existing data sources for scheduling also get model and version information. But we haven't distinguished so far between the battery scheduler and the charging-station scheduler. With this PR, we would start doing that, but I don't believe we can automate for people how to separate scheduling data attribution in their existing data. The way forward I see now is to
How would you go forward with this? |
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.
Nicely done. Only a few small comments left, and one possible problem (with searching beliefs with source=scheduler_sources[-1]
).
And are you sure we shouldn't go with scheduler classes instead of functions, before opening up scheduler customisation to users?
|
I wanted to make another PR for that before the next release, and have postponed the exact decision. |
…al data source ID for this lookup if possible Signed-off-by: Nicolas Höning <nicolas@seita.nl>
Sorry, I had missed this comment before. Such a data migration would make sense imo, but it's not really mandatory so we could use an opt-in parameter for the upgrade command in the revision file (we had some previous revisions that used parameters, which could serve as an example). The relevant |
Yes I found out about them, as well. Is your idea that the parameter decides which data source to use? |
schedule_battery and schedule_charging_station 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>
…torage-specs # Conflicts: # documentation/changelog.rst
…: 3' over 'version: v3'. Even though Scheduler versioning does not necessarily need to follow semantic versioning (see discussion here: semver/semver#235), the v is still redundant. Signed-off-by: F.N. Claessen <felix@seita.nl>
Handing it over to you again.
|
Thanks! I highly doubt a plugin would use them, but good thinking. |
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>
…ero power values for missing values Signed-off-by: F.N. Claessen <felix@seita.nl>
Maybe merging this PR at this stage would be nice, and I add the parameterization update for the trigger endpoint in a separate PR. This PR is huge, and you just added a few smaller things, as well. I just reviewed those fixes and am okay with them. Do you need to do any reviewing here or are we done? The changelog message could maybe be expanded to include that we refactored basic scheduling code, as well, but it's not that interesting to users. |
It might be good to mention that we merged our two scheduler functions (for batteries and Charge Points) into a single scheduler class.
You're right, that is still missing. |
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
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.
Looks great. I have two easily fixed remarks left. Otherwise, I approve.
flexmeasures/data/migrations/versions/650b085c0ad3_consolidate_data_source_after_storage_.py
Show resolved
Hide resolved
flexmeasures/data/migrations/versions/650b085c0ad3_consolidate_data_source_after_storage_.py
Show resolved
Hide resolved
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
…asures/flexmeasures into refactor-scheduling-storage-specs
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
This PR adds documentation to the /api/sensor//schedules/trigger endpoint to better understand how to describe a flexibility model. Also in preparation for more flexibility modes to come, right now we only support storage.
The code also gets a small refactoring to make this clearer to developers.
Finally, we merge two storage-based schedulers (battery & charging station) into one.