Allow parameters to be defined as timeseries for storage assets by MaGering · Pull Request #720 · rl-institut/multi-vector-simulator

MaGering · 2020-12-16T11:10:13Z

With this PR it is possible to process a time series of the Storage component passed in storage_*.csv.

In energyStorage(...) of C0_data_processing.py the key 'value' leads to an error because it does not exists before the time series is read. I therefore replaced it with another condition, which is analogous to the parameter 'efficiency' of the Transformer component and adapted it accordingly:

(
FILENAME in dict_values[group][asset][subasset][parameter]
and HEADER in dict_values[group][asset][subasset][parameter]
):

The following steps were realized, as well (if applies):

Use in-line comments to explain your code
Write docstrings to your code (example docstring)
For new functionalities: Explain in readthedocs
Write test(s) for your new patch of code (pytests, assertion debug messages)
Update the CHANGELOG.md
Apply black (black . --exclude docs/)
Check if benchmark tests pass locally (EXECUTE_TESTS_ON=master pytest)

Please mark above checkboxes as following:

Open
Done

❌ Check not applicable to this PR

_{For more information on how to contribute check the CONTRIBUTING.md.}

MaGering · 2020-12-16T11:15:55Z

Unfortunately one check is failing. Do I need to have python 3.8 installed? I don't really understand the error message. Could you give me a hint @smartie2076?

smartie2076 · 2020-12-16T11:38:55Z

Hi @MaGering!

As a tip, instead of writing

Fix #Issue #719 [as a link]

Please write

Fix #719

(It seems that github understands that you want to mention the issue, but it does not connect those two in this fashion. Also, by writing Fix #issue and only then the number, the issue will not be closed automatically.

MaGering · 2020-12-16T11:40:46Z

Hi @MaGering!

As a tip, instead of writing

Fix #Issue #719 [as a link]

Please write

Fix #719

(It seems that github understands that you want to mention the issue, but it does not connect those two in this fashion. Also, by writing Fix #issue and only then the number, the issue will not be closed automatically.

Oh right! Thanks for this advise. I will change that!

- Exception needs to be tested - Changed `energyStorage.csv` - Added `storage_1.csv` - Added column in `parameter_timeseries`

smartie2076 · 2020-12-16T11:56:57Z

I am checking locally now if that fix works.

smartie2076

This works as a quick fix for the issue. However, this is not really the proper way to do it - we would need to change A1 for that.

Current result:

"soc_min": {
    "file_name": "parameter_timeseries.csv",
    "header": "soc_min",
    "unit": "factor",
    "value": [...]
},

filename, unit and header should, however, be wrapped within value as an output of A1. As this is more work, we can leave it at that right now.

Necessary is, however, that you create a CHANGELOG.md entry. Please onle make your PR ready to merge/review green if you are sure it can be merged (see checklist), ie. initially always create a draft pull request.

Will request a PR into this one with additions.

MaGering · 2020-12-16T12:58:47Z

This works as a quick fix for the issue. However, this is not really the proper way to do it - we would need to change A1 for that.

Current result:
"soc_min": {
    "file_name": "parameter_timeseries.csv",
    "header": "soc_min",
    "unit": "factor",
    "value": [...]
},
filename, unit and header should, however, be wrapped within value as an output of A1. As this is more work, we can leave it at that right now.

I must admit I don't quite understand this. I checked for the Transformer's (heat pump) efficiency and it's the same structure there:

What you can see in the picture is, that filename, unit and header are not wrapped within value either. For solving this in future I think it is important to find out if it is a general issue for every group or specific for Transformer and Storage.

However I'm glad that I can work with this solution for now. :)

smartie2076 · 2020-12-16T13:32:05Z

This works as a quick fix for the issue. However, this is not really the proper way to do it - we would need to change A1 for that.
What you can see in the picture is, that filename, unit and header are not wrapped within value either. For solving this in future I think it is important to find out if it is a general issue for every group or specific for Transformer and Storage.

Oh, really?! Then I might have been mistaken. Even better!

…ries_review

…eview Fix/c0 storage time series review

smartie2076

Ready to be merged :)

smartie2076 · 2020-12-18T10:36:58Z

src/multi_vector_simulator/C0_data_processing.py

+                if parameter in dict_values[group][asset][subasset] and (
+                    FILENAME in dict_values[group][asset][subasset][parameter]
+                    and HEADER in dict_values[group][asset][subasset][parameter]


Usually, I would request a pytest to be created for this case. As this feature is covered with the benchmark test I created, and C0 functions are anyway lacking pytests, this is fine.

MaGering added 2 commits December 16, 2020 11:52

Fix KeyError: 'value' if times series is passed

9a1bd9b

Apply black

a8a47d5

MaGering requested a review from smartie2076 December 16, 2020 11:13

Add storage with soc_min defined as timeseries

79acb01

- Exception needs to be tested - Changed `energyStorage.csv` - Added `storage_1.csv` - Added column in `parameter_timeseries`

MaGering mentioned this pull request Dec 16, 2020

[Bug] Storage component can not process time series #719

Closed

5 tasks

smartie2076 reviewed Dec 16, 2020

View reviewed changes

smartie2076 added 2 commits December 16, 2020 13:31

Adapt benchmark test

cf15bfc

Changelog.md

d6c44fa

smartie2076 mentioned this pull request Dec 16, 2020

Fix/c0 storage time series review #723

Merged

MaGering and others added 5 commits December 17, 2020 12:50

Merge branch 'dev' into fix/C0_storage_time_series

416a0e2

Merge branch 'fix/C0_storage_time_series' into fix/C0_storage_time_se…

d5d9483

…ries_review

Merge pull request #723 from rl-institut/fix/C0_storage_time_series_r…

5a41da7

…eview Fix/c0 storage time series review

Apply black on tests/test_benchmark_special_features.py

5075ed5

Update CHANGELOG.md

efbd151

MaGering requested a review from smartie2076 December 17, 2020 16:16

Merge branch 'dev' into fix/C0_storage_time_series

83a38dd

smartie2076 changed the title ~~Fix KeyError: 'value' if times series is passed~~ Allow parameters to be defined as timeseries for storage assets Dec 18, 2020

smartie2076 approved these changes Dec 18, 2020

View reviewed changes

smartie2076 mentioned this pull request Dec 18, 2020

Release v.0.5.4 #734

Closed

1 task

Merge branch 'dev' into fix/C0_storage_time_series

d2257e5

MaGering merged commit 4fcdb51 into dev Dec 18, 2020

MaGering deleted the fix/C0_storage_time_series branch December 18, 2020 14:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow parameters to be defined as timeseries for storage assets#720

Allow parameters to be defined as timeseries for storage assets#720
MaGering merged 12 commits intodevfrom
fix/C0_storage_time_series

MaGering commented Dec 16, 2020 •

edited

Loading

Uh oh!

MaGering commented Dec 16, 2020

Uh oh!

smartie2076 commented Dec 16, 2020 •

edited

Loading

Uh oh!

MaGering commented Dec 16, 2020

Uh oh!

smartie2076 commented Dec 16, 2020

Uh oh!

smartie2076 left a comment

Uh oh!

MaGering commented Dec 16, 2020

Uh oh!

smartie2076 commented Dec 16, 2020

Uh oh!

smartie2076 left a comment

Uh oh!

smartie2076 Dec 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

MaGering commented Dec 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaGering commented Dec 16, 2020

Uh oh!

smartie2076 commented Dec 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaGering commented Dec 16, 2020

Uh oh!

smartie2076 commented Dec 16, 2020

Uh oh!

smartie2076 left a comment

Choose a reason for hiding this comment

Uh oh!

MaGering commented Dec 16, 2020

Uh oh!

smartie2076 commented Dec 16, 2020

Uh oh!

smartie2076 left a comment

Choose a reason for hiding this comment

Uh oh!

smartie2076 Dec 18, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

MaGering commented Dec 16, 2020 •

edited

Loading

smartie2076 commented Dec 16, 2020 •

edited

Loading