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

Document & refactor scheduling specs for storage flexibility model #511

Merged
merged 41 commits into from Nov 18, 2022

Conversation

nhoening
Copy link
Contributor

@nhoening nhoening commented Oct 1, 2022

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.

@coveralls
Copy link
Collaborator

coveralls commented Oct 1, 2022

Pull Request Test Coverage Report for Build 3494526674

  • 158 of 182 (86.81%) changed or added relevant lines in 13 files are covered.
  • 4 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.05%) to 64.974%

Changes Missing Coverage Covered Lines Changed/Added Lines %
flexmeasures/api/common/schemas/sensor_data.py 1 2 50.0%
flexmeasures/cli/data_add.py 1 2 50.0%
flexmeasures/data/models/planning/init.py 4 5 80.0%
flexmeasures/data/models/planning/storage.py 59 60 98.33%
flexmeasures/data/services/scheduling.py 44 48 91.67%
flexmeasures/data/models/planning/battery.py 0 5 0.0%
flexmeasures/data/models/planning/charging_station.py 0 5 0.0%
flexmeasures/data/models/planning/utils.py 24 30 80.0%
Files with Coverage Reduction New Missed Lines %
flexmeasures/api/common/schemas/sensor_data.py 1 66.84%
flexmeasures/api/v1_3/implementations.py 1 67.53%
flexmeasures/api/v3_0/sensors.py 2 69.95%
Totals Coverage Status
Change from base Build 3487808393: -0.05%
Covered Lines: 6583
Relevant Lines: 9511

💛 - Coveralls

@nhoening nhoening changed the base branch from main to Issue-9-custom-scheduler October 1, 2022 16:29
@nhoening nhoening requested a review from Flix6x October 11, 2022 20:24
Base automatically changed from Issue-9-custom-scheduler to main October 16, 2022 10:35
Copy link
Contributor

@Flix6x Flix6x left a 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.

flexmeasures/api/v3_0/sensors.py Outdated Show resolved Hide resolved
flexmeasures/api/v3_0/sensors.py Outdated Show resolved Hide resolved
flexmeasures/api/v3_0/sensors.py Outdated Show resolved Hide resolved
flexmeasures/api/v3_0/sensors.py Outdated Show resolved Hide resolved
flexmeasures/data/services/scheduling.py Outdated Show resolved Hide resolved
flexmeasures/data/models/planning/utils.py Outdated Show resolved Hide resolved
flexmeasures/data/models/planning/utils.py Outdated Show resolved Hide resolved
flexmeasures/data/models/planning/utils.py Outdated Show resolved Hide resolved
flexmeasures/api/v3_0/sensors.py Outdated Show resolved Hide resolved
flexmeasures/data/models/planning/tests/test_solver.py Outdated Show resolved Hide resolved
…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>
@nhoening nhoening force-pushed the refactor-scheduling-storage-specs branch from 3255c89 to c0400ae Compare October 29, 2022 18:41
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>
documentation/changelog.rst Outdated Show resolved Hide resolved
flexmeasures/api/v3_0/sensors.py Outdated Show resolved Hide resolved
flexmeasures/data/models/planning/utils.py Outdated Show resolved Hide resolved
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>
@nhoening nhoening requested a review from Flix6x October 31, 2022 23:33
This was referenced Nov 1, 2022
@nhoening
Copy link
Contributor Author

nhoening commented Nov 2, 2022

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

  • make an existing source with name "Seita" and type "scheduling script" the battery one (model "schedule_battery", version "1")
  • add another one for model = "schedule_charging_station"
  • Any existing scheduling data will still be linked to the first source. There is a potential SQL Update command which changes the source for scheduling data for all those sensors which actually represent a charging station (but I don't think there is a generic_asset_type which would help us).

How would you go forward with this?

Copy link
Contributor

@Flix6x Flix6x left a 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]).

flexmeasures/api/v1_3/implementations.py Outdated Show resolved Hide resolved
flexmeasures/api/v1_3/implementations.py Outdated Show resolved Hide resolved
flexmeasures/api/v3_0/sensors.py Outdated Show resolved Hide resolved
flexmeasures/api/v3_0/tests/test_sensor_schedules.py Outdated Show resolved Hide resolved
flexmeasures/data/models/planning/utils.py Outdated Show resolved Hide resolved
flexmeasures/data/services/scheduling.py Outdated Show resolved Hide resolved
flexmeasures/data/services/scheduling.py Outdated Show resolved Hide resolved
flexmeasures/data/services/scheduling.py Outdated Show resolved Hide resolved
@Flix6x
Copy link
Contributor

Flix6x commented Nov 2, 2022

And are you sure we shouldn't go with scheduler classes instead of functions, before opening up scheduler customisation to users?

class StorageScheduler(BaseScheduler):
    __author__ = "Organization"
    __version__ = 1

    def schedule(sensor, start, end, resolution, storage_specs):

@nhoening
Copy link
Contributor Author

nhoening commented Nov 2, 2022

And are you sure we shouldn't go with scheduler classes instead of functions, before opening up scheduler customisation to users?

class StorageScheduler(BaseScheduler):
    __author__ = "Organization"
    __version__ = 1

    def schedule(sensor, start, end, resolution, storage_specs):

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>
@Flix6x
Copy link
Contributor

Flix6x commented Nov 2, 2022

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.
...
How would you go forward with this?

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 GenericAssetType.name would be "one-way_evse" and "two-way_evse" btw.

@nhoening
Copy link
Contributor Author

nhoening commented Nov 2, 2022

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>
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>
@Flix6x
Copy link
Contributor

Flix6x commented Nov 10, 2022

Handing it over to you again.

  • Merged PR Support pandas 1.4 #525 to ensure our left-inclusive DatetimeIndex works with pandas>=1.4
  • Our schedulers are merged into StorageScheduler and moved it to a new module
  • Original scheduling functions placed back with deprecation warnings (in case plugins use them, to help them transition)

@nhoening
Copy link
Contributor Author

Thanks! I highly doubt a plugin would use them, but good thinking.

@Flix6x Flix6x added this to the 0.12.0 milestone Nov 10, 2022
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>
@nhoening
Copy link
Contributor Author

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.

@nhoening
Copy link
Contributor Author

nhoening commented Nov 11, 2022

The changelog message could maybe be expanded

It might be good to mention that we merged our two scheduler functions (for batteries and Charge Points) into a single scheduler class.

Do we still need a migration, where we amend the data source for existing schedules? (all using the StorageScheduler)?

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>
@nhoening nhoening requested a review from Flix6x November 16, 2022 06:27
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
@nhoening nhoening self-assigned this Nov 17, 2022
Copy link
Contributor

@Flix6x Flix6x left a 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.

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>
@nhoening nhoening merged commit 7715219 into main Nov 18, 2022
@nhoening nhoening deleted the refactor-scheduling-storage-specs branch November 18, 2022 07:50
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.

None yet

3 participants