Fix/transformer component definition by SabineHaas · Pull Request #596 · rl-institut/multi-vector-simulator

SabineHaas · 2020-10-08T08:44:29Z

Fix #333
Fix #389
Fix #246

Changes proposed in this pull request:

All variable_costs, efficiency and nominal_value of transformers on output flows
Add and fix tests

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

…s on output flows

SabineHaas · 2020-10-08T08:58:58Z

I've just realized that in the documentation there's an example for an electrolyzer that works differently.. maybe it's worth to talk about it. Let me know when you have time @smartie2076

SabineHaas · 2020-10-08T12:07:51Z

I've just realized that in the documentation there's an example for an electrolyzer that works differently.. maybe it's worth to talk about it. Let me know when you have time @smartie2076

moved to issue #599
So, this PR is ready for review.

smartie2076

I did go commit by commit now to ease the process, but I am not sure how comments are presented when you afterwards changed the specific parts of code I commented on. There may be outdated comments.

Commit ebece71 was a bit big to be reviewed.

What do you mean with "cry for benchmark tests" in b292bd4?

Can you add more info on how transformers are modeled and what the parameters mean in the RTD? Both in Modell_Assumptions.rst and in the parameter description for efficiency as a specific case. Do you have knowledge to share how to use multilple in/outputs?

CHANGELOG.md

smartie2076 · 2020-10-21T09:45:36Z

tests/test_data/inputs_for_D1/mvs_config.json

                "value": 10
            }
        },
+        "transformer_optimize_multiple_output_busses": {


A comment to this file: I like to define this dict directly in the pytest files, because then we do not need to check if the stuff around it is important. Not sure if that works for you, though.

In the beginning I defined only needed data in the json file, which was already a lot, which is why I didn't define the dictionaries in the python file. You have added the information around once (was necessary for other tests / completeness of input data.... I don't remember well). So, now I'm a bit confused - I thought the stuff around was necessary - would you remove it again?

@Bachibouzouk correct me if I am wrong, but I think if it is a json file, then the json must be a complete input file with all parameters. If you define it in the pytest directly, then it does not have to be complete. If all of that is needed for the test/it is too much to define in the py file, then we should leave it as is.

Not all of it is needed, but also before entries were added to make the json complete it was too much to define in the py file, to my mind. So, I would leave it as it is.

src/multi_vector_simulator/D1_model_components.py

tests/test_D1_model_components.py

smartie2076 · 2020-10-21T10:17:07Z

Ah, and in the D1.transfomer functions (and the subsequent ones), can you add under Notes in the docstrings, with which pytests the function is tested?

smartie2076 · 2020-10-21T10:33:55Z

Executing all pytests locally results in an error:

[...]

======================================================================== FAILURES =========================================================================
________________________________ test_input_folder_json_file_have_required_parameters[d:\\pycharmprojects\\mvs_eland\\in] _________________________________

input_folder = 'd:\\pycharmprojects\\mvs_eland\\in'

    @pytest.mark.parametrize("input_folder", TEST_JSON_INPUT_FOLDERS)
    def test_input_folder_json_file_have_required_parameters(input_folder):
        """
        Browse all folders which contains a {CSV_ELEMENTS} folder and a {JSON_FNAME} json file
        (defined as json input folders) and verify that this json file have all required
        parameters
        """
        if os.path.exists(os.path.join(input_folder, JSON_FNAME)):
>           comparison = compare_input_parameters_with_reference(input_folder, ext=JSON_EXT)

tests\test_input_folder_parameters.py:72:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
src\multi_vector_simulator\utils\__init__.py:111: in compare_input_parameters_with_reference
    main_parameters = json.load(fp)
win_venv\lib\json\__init__.py:296: in load
    parse_constant=parse_constant, object_pairs_hook=object_pairs_hook, **kw)
win_venv\lib\json\__init__.py:348: in loads
    return _default_decoder.decode(s)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <json.decoder.JSONDecoder object at 0x00000285F077FD08>
s = '{\n    "constraints": {\n        "minimal_renewable_share": {\n            "unit": "factor",\n            "value": 0....e": false\n        },\n\t\t"tim
estep": {\n            "unit": "minutes",\n            "value": 60\n        }\n    }\n}'
_w = <built-in method match of re.Pattern object at 0x00000285F0742C30>

    def decode(self, s, _w=WHITESPACE.match):
        """Return the Python representation of ``s`` (a ``str`` instance
        containing a JSON document).

        """
        obj, end = self.raw_decode(s, idx=_w(s, 0).end())
        end = _w(s, end).end()
        if end != len(s):
>           raise JSONDecodeError("Extra data", s, end)
E           json.decoder.JSONDecodeError: Extra data: line 219 column 10 (char 7483)

win_venv\lib\json\decoder.py:340: JSONDecodeError

[...]

================================================================= short test summary info =================================================================
FAILED tests/test_input_folder_parameters.py::test_input_folder_json_file_have_required_parameters[d:\\pycharmprojects\\mvs_eland\\in] - json.decoder.JSO...

============================================ 1 failed, 276 passed, 2 skipped, 15 warnings in 263.59s (0:04:23) ============================================

SabineHaas · 2020-10-26T15:55:20Z

I did go commit by commit now to ease the process, but I am not sure how comments are presented when you afterwards changed the specific parts of code I commented on. There may be outdated comments.

Commit ebece71 was a bit big to be reviewed.

What do you mean with "cry for benchmark tests" in b292bd4?

I think it doesn't make sense to check the creation of transformers with efficiency as time series for all cases all over again - I thought if we want to test this we would have to do it in a benchmark test.

Can you add more info on how transformers are modeled and what the parameters mean in the RTD? Both in Modell_Assumptions.rst

added

and in the parameter description for efficiency as a specific case.

It already says "Ratio of energy output/energy input." there, this accounts for a transformer, as well.

Do you have knowledge to share how to use multilple in/outputs?

Only how to define them, as explained here. I haven't used a transformer with multiple in- or outflows, yet.

SabineHaas · 2020-10-26T16:57:24Z

Executing all pytests locally results in an error:

I don't get any error. But also didn't before I changed parts of the code - so maybe you should run the tests again to be sure.

Bachibouzouk · 2020-10-26T17:30:11Z

'd:\pycharmprojects\mvs_eland\in'

@smartie2076 , the error is only local because it looks into one of the input files it found in your path (cf above).

when tests/test_input_folder_parameters.py fails you have to look at which folder is in there, it might be (like here) a local folder

smartie2076 · 2020-11-04T11:02:36Z

I think it doesn't make sense to check the creation of transformers with efficiency as time series for all cases all over again - I thought if we want to test this we would have to do it in a benchmark test.

It also is in a benchmark test, I believe.

Do you have knowledge to share how to use multilple in/outputs?

Only how to define them, as explained here. I haven't used a transformer with multiple in- or outflows, yet.

Acknowledged, we can see when implementing more benchmark tests.

smartie2076

Thank you! If I am right, you only had to add assertion messages and notes where you test which functions. Thats all good!

I would add the sentence about the changed D1 functions in the changelog.md, but yeah...

SabineHaas added 5 commits October 8, 2020 10:38

Delete todo as this is described in the documentation

495cf18

All variable_costs, efficiency and nominal_value of transformer…

ebece71

…s on output flows

Delete test func names - they cry for benchmark tests

b292bd4

Update changelog

040194d

Add necessary inputs for D1.transformer() tests

42868d1

SabineHaas requested a review from smartie2076 October 8, 2020 08:57

SabineHaas self-assigned this Oct 8, 2020

SabineHaas mentioned this pull request Oct 8, 2020

Adapt D1 transformer functions to allow multiple inputs (f.e. for electrolyzers) #599

Closed

smartie2076 approved these changes Oct 21, 2020

View reviewed changes

SabineHaas added 3 commits October 26, 2020 16:42

Remove unnecessary variable index

4ee30b2

Add transformer into to docs (model assumptions)

fad47f1

Update changelog

d22c1c7

SabineHaas added 3 commits October 26, 2020 17:19

Add test function names to Notes sections of functions in D1

42aa1b0

Merge branch 'dev' into fix/transformer_component_definition

21ae71e

Add assertion messages to tranformer tests

77ba1ae

smartie2076 approved these changes Nov 4, 2020

View reviewed changes

SabineHaas added 2 commits November 4, 2020 16:21

Update changelog

57ef151

Merge branch 'dev' into fix/transformer_component_definition

da10274

smartie2076 merged commit 6a7286b into dev Nov 5, 2020

smartie2076 deleted the fix/transformer_component_definition branch November 5, 2020 09:51

smartie2076 mentioned this pull request Nov 10, 2020

Release/v0.5.1 #648

Merged

SabineHaas mentioned this pull request Nov 12, 2020

Update to MVS pypi release greco-project/pvcompare#142

Closed

6 tasks

Conversation

SabineHaas commented Oct 8, 2020 • edited by smartie2076 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SabineHaas commented Oct 8, 2020

Uh oh!

SabineHaas commented Oct 8, 2020

Uh oh!

smartie2076 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

smartie2076 Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

SabineHaas Oct 26, 2020

Choose a reason for hiding this comment

Uh oh!

smartie2076 Nov 4, 2020

Choose a reason for hiding this comment

Uh oh!

SabineHaas Nov 4, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smartie2076 commented Oct 21, 2020

Uh oh!

smartie2076 commented Oct 21, 2020

Uh oh!

SabineHaas commented Oct 26, 2020

Uh oh!

SabineHaas commented Oct 26, 2020

Uh oh!

Bachibouzouk commented Oct 26, 2020

Uh oh!

smartie2076 commented Nov 4, 2020

Uh oh!

smartie2076 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SabineHaas commented Oct 8, 2020 •

edited by smartie2076

Loading