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

Fix/c0 storage time series review #723

Merged

Conversation

smartie2076
Copy link
Collaborator

Here are my adaptations. I did neither do a linting test with black nor executed all pytests - so please do that before you request a new review for your PR #720.

In your PR, please also add the correct number for the benchmark tests changelog entry.

- Exception needs to be tested
- Changed `energyStorage.csv`
- Added `storage_1.csv`
- Added column in `parameter_timeseries`
Comment on lines +37 to +38
- Changed `C0.energyStorage()` for timeseries in storage parameters (hotfix) (#720)
- Input files and benchmark test `test_benchmark_special_features.Test_Parameter_Parsing()`: Now also including timeseries in a storage component ()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @MaGering !

"In your PR, please also add the correct number for the benchmark tests changelog entry."

Refers to the lines above. I added changelog entries for what you changed, where I also put your PR as origin (#720). Line 38 is what I changed in this PR (#723). As I can not know what the PR number is when I am working on my lokal branch, I couldnt assign the number to the changelog entry. Please add the number in the brackets ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can change this either locally, or by opening the file view of the changelog in github, making a manual change, and submitting it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I will do the changes in my PR, correct?

Copy link
Collaborator

@MaGering MaGering left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! The benchmark test ran without error as I checked it locally. Only test_benchmark_special_features.py needs to be reformatted with black and conflicts with CHANGELOG.md resolved.

@smartie2076
Copy link
Collaborator Author

Looks good to me! The benchmark test ran without error as I checked it locally. Only test_benchmark_special_features.py needs to be reformatted with black and conflicts with CHANGELOG.md resolved.

Nice! As you are merging into a PR and not into dev can solve the merge conflicts now, merge, and then run black on fix/C0_storage_time_series.

@MaGering MaGering merged commit 5a41da7 into fix/C0_storage_time_series Dec 17, 2020
@MaGering MaGering deleted the fix/C0_storage_time_series_review branch December 17, 2020 15:58
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