Create a new parameter for scenario description by mahendrark · Pull Request #722 · rl-institut/multi-vector-simulator

mahendrark · 2020-12-16T11:54:19Z

Fix #184

Changes proposed in this pull request:

Add a new parameter scenario_description to the input files
Add this parameter to the sections in RTD discussing MVS parameters
Add a section in autoreport describing the current scenario being simulated

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.}

mahendrark · 2020-12-18T14:14:54Z

@smartie2076

It works as intended. See the attached simulation report below. I added the scenario description to the report.

simulation_report.pdf

smartie2076 · 2020-12-18T14:26:59Z

@smartie2076

It works as intended. See the attached simulation report below. I added the scenario description to the report.

simulation_report.pdf

Oh neat! Great idea to add an additional header :)

I will not be able to review this today. @SabineHaas if you like the feature for pvcompare, you could consider this.

SabineHaas · 2020-12-18T15:35:52Z

@smartie2076
It works as intended. See the attached simulation report below. I added the scenario description to the report.
simulation_report.pdf

Oh neat! Great idea to add an additional header :)

I will not be able to review this today. @SabineHaas if you like the feature for pvcompare, you could consider this.

sorry I haven't seen that before the release

smartie2076 · 2020-12-18T15:42:32Z

I will not be able to review this today. @SabineHaas if you like the feature for pvcompare, you could consider this.

sorry I haven't seen that before the release

That's fine, did not expect you to include it anyway. Fyi: in the beginning of January we will fix a JSON input file for the EPA, so from there on out all new parameters must not require an API change anymore. Possible great news for @Piranias, especially if we don't implement this in the MVS-EPA parser but require this for the CSV files as well.

mahendrark · 2021-01-05T18:12:17Z

@smartie2076

One of the tests is failing. I did not write any tests in this, nor did I touch any of those tests. Can you please take a look at it? Otherwise, the PR is done.

smartie2076

Hi @mahendrark!

There should be many more files that need to be adapted - see folders input_template and tests/benchmark_test_inputs. Also, you should specifically search for .json input files - there, the default parameters are not added from EXTRA_CSV_PARAMETERS (this is only applied when reading csv, not json as an input file). This is the reason for one of the test failures:

E           AssertionError: In path /home/runner/work/multi-vector-simulator/multi-vector-simulator/tests/benchmark_test_inputs/rerun/mvs_config.json, the following parameters are missing:{'project_data': ['scenario_description']}
E           assert 'missing_parameters' not in {'extra_parameters': {'economic_data': ['crf', 'annuity_factor'], 'paths_to_plots': [], 'project_data': ['sectors'], '...er', 'path_input_folder', 'end_date', 'time_index']}, 'missing_parameters': {'project_data': ['scenario_description']}}

Can you create an issue that mentions this? For that, please also try running /home/runner/work/multi-vector-simulator/multi-vector-simulator/tests/benchmark_test_inputs/rerun/mvs_config.json with the MVS (-ext json), and add the error message. Post the json file into the issue as well as you manually fixed version.

docs/MVS_parameters_list.csv

tests/test_data/inputs_for_D0/mvs_config.json

tests/test_data/inputs_for_D1/mvs_config.json

tests/inputs/mvs_config.json

smartie2076 · 2021-01-06T09:41:53Z

Regarding the other pytest error

FAILED tests/test_benchmark_KPI.py::Test_Economic_KPI::test_benchmark_Economic_KPI_C2_E2

I suspect that this might be an issue with the defined datatype/unit which should be string.

value = ''
asset_dict = {'country': 'A_country', 'latitude': '0', 'longitude': '0', 'project_id': '1', ...}
row = unit            scenario_description
project_data                        
Name: scenario_description, dtype: object
param = 'scenario_description', asset = 'project_data'
filename = 'project_data'
...
>                       value = int(value)
 E                       ValueError: invalid literal for int() with base 10: ''

It seems to think that the string of scenario_description is to be interpreted as a int. I do not know why this should only be the case for this specific benchmark test (did you try with the other benchmark tests locally?)...

mahendrark · 2021-01-06T19:48:57Z

@smartie2076 all problems are fixed now. Ready for review :)))

mahendrark · 2021-01-07T08:17:48Z

Commits are more meaningful now as well!

docs/MVS_parameters_list.csv

CHANGELOG.md

tests/benchmark_test_inputs/AD_grid_diesel/csv_elements/project_data.csv

tests/inputs/csv_elements/project_data.csv

Bachibouzouk

Nice job @mahendrark

@smartie2076 would the change to docs/MVS_parameters_list.csv cause trouble in dev? This is the last thing we would prevent me from merging this one now ;)

mahendrark · 2021-01-11T09:17:27Z

@Bachibouzouk

Something strange happened to this branch when I tried to pull your changes. I solved the merge conflicts and committed it and then now it says I have 19 commits. Can you please take a look and tell me how to fix this?

smartie2076 · 2021-01-11T09:50:28Z

@smartie2076 would the change to docs/MVS_parameters_list.csv cause trouble in dev? This is the last thing we would prevent me from merging this one now ;)

I think it would be better to delete the first numbered column of the file first in a new PR, merge that into dev and then merge into this one. We have had 4 or 5 PRs now that changed the file, and each may corrupt it or revert the previous changes :´(

Other problem: When you merge this, you need to include it in the parser of #675, as we do not add the default values for new parameters when reading from json (#750).

Bachibouzouk · 2021-01-11T10:01:50Z

Other problem: When you merge this, you need to include it in the parser of #675, as we do not add the default values for new parameters when reading from json (#750).

It is already included :)

Bachibouzouk · 2021-01-11T10:54:26Z

After the tests pass @mahendrark you can merge this PR :)

mahendrark marked this pull request as draft December 16, 2020 11:54

mahendrark force-pushed the feature/scenario_desc branch 3 times, most recently from 495d7be to aa3216c Compare December 18, 2020 14:08

mahendrark requested a review from smartie2076 December 18, 2020 14:12

mahendrark marked this pull request as ready for review December 18, 2020 14:13

mahendrark added the enhancement New feature or request label Dec 18, 2020

mahendrark force-pushed the feature/scenario_desc branch from f55fdcd to 636d809 Compare December 18, 2020 14:17

mahendrark requested review from SabineHaas and removed request for smartie2076 December 18, 2020 14:24

mahendrark force-pushed the feature/scenario_desc branch 2 times, most recently from a43f558 to 848005b Compare December 18, 2020 16:21

mahendrark requested a review from smartie2076 December 23, 2020 12:14

mahendrark force-pushed the feature/scenario_desc branch 2 times, most recently from 197b0a3 to 18615b0 Compare January 5, 2021 18:01

mahendrark removed the request for review from SabineHaas January 5, 2021 18:02

mahendrark force-pushed the feature/scenario_desc branch from 18615b0 to e6a261b Compare January 5, 2021 18:05

smartie2076 reviewed Jan 6, 2021

View reviewed changes

docs/MVS_parameters_list.csv Outdated Show resolved Hide resolved

tests/test_data/inputs_for_D0/mvs_config.json Show resolved Hide resolved

tests/test_data/inputs_for_D1/mvs_config.json Show resolved Hide resolved

tests/inputs/mvs_config.json Show resolved Hide resolved

mahendrark mentioned this pull request Jan 6, 2021

Failing tests in the CI builds #749

Closed

mahendrark self-assigned this Jan 6, 2021

mahendrark requested a review from smartie2076 January 6, 2021 19:48

mahendrark force-pushed the feature/scenario_desc branch from 84c1dcc to a45dfed Compare January 7, 2021 08:17

smartie2076 mentioned this pull request Jan 7, 2021

No check of completeness or default values for json input keys implemented #750

Open

7 tasks

Bachibouzouk force-pushed the feature/scenario_desc branch from a45dfed to d3fe7fd Compare January 8, 2021 22:15

Bachibouzouk reviewed Jan 8, 2021

View reviewed changes

docs/MVS_parameters_list.csv Outdated Show resolved Hide resolved

Bachibouzouk reviewed Jan 8, 2021

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

Bachibouzouk reviewed Jan 8, 2021

View reviewed changes

tests/benchmark_test_inputs/AD_grid_diesel/csv_elements/project_data.csv Show resolved Hide resolved

Bachibouzouk reviewed Jan 8, 2021

View reviewed changes

tests/inputs/csv_elements/project_data.csv Show resolved Hide resolved

Bachibouzouk mentioned this pull request Jan 8, 2021

xlrd==1.2.0 #744

Merged

5 tasks

Bachibouzouk approved these changes Jan 8, 2021

View reviewed changes

Bachibouzouk force-pushed the feature/scenario_desc branch from dadcfae to d3fe7fd Compare January 11, 2021 09:27

mahendrark added 8 commits January 11, 2021 10:30

Add new parameter to hold scenario description from user

03af7d2

Update all project_data.csv files with scenario_description parameter

033f646

Update the .json input files with the scenario_description parameter

7d0ca64

Add constants for the newly added parameter scenario_description

2024250

Add scenario description section to the autoreport

d107e64

Update the input_template with the scenario_description parameter

4104878

Add the scenario_description parameter to parameters description in docs

e804e28

Update CHANGELOG

98b38cc

Bachibouzouk force-pushed the feature/scenario_desc branch from d3fe7fd to 98b38cc Compare January 11, 2021 09:31

Bachibouzouk mentioned this pull request Jan 11, 2021

Fix/adapt epa parser #675

Merged

11 tasks

Remove the index in the MVS_parameters_list.csv

8f14a31

mahendrark merged commit 8e789b4 into dev Jan 11, 2021

mahendrark deleted the feature/scenario_desc branch January 11, 2021 11:03

smartie2076 mentioned this pull request Jan 12, 2021

[Bug] EXTRA_CSV_PARAMETERS can not be used to add default values for non-unit-value-pairs #755

Closed

5 tasks

MaGering mentioned this pull request Jan 14, 2021

With mvs release 0.5.5 new parameter scenario_description is added greco-project/pvcompare#205

Closed

Conversation

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

Uh oh!

Uh oh!

mahendrark commented Dec 18, 2020

Uh oh!

smartie2076 commented Dec 18, 2020

Uh oh!

SabineHaas commented Dec 18, 2020

Uh oh!

smartie2076 commented Dec 18, 2020

Uh oh!

mahendrark commented Jan 5, 2021

Uh oh!

smartie2076 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smartie2076 commented Jan 6, 2021

Uh oh!

mahendrark commented Jan 6, 2021

Uh oh!

mahendrark commented Jan 7, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Bachibouzouk left a comment

Choose a reason for hiding this comment

Uh oh!

mahendrark commented Jan 11, 2021

Uh oh!

smartie2076 commented Jan 11, 2021

Uh oh!

Bachibouzouk commented Jan 11, 2021

Uh oh!

Bachibouzouk commented Jan 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mahendrark commented Dec 16, 2020 •

edited

Loading