-
Notifications
You must be signed in to change notification settings - Fork 9
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
first draft - Linking transformer stations #156
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @remmyd! I dont have the time to still check the actual code (and if it works), but so far it looks good :)
There are some files that should not have been commited to this branch, namely the .log
files and all .pyc
files. You need to remove those files from the PR.
I would look into pytests next, try something simple like in B
to start with. At the end, you should have a benchmark test that makes sure that your constraint works.
@@ -414,6 +415,12 @@ def get_case_definitions(file, sheet_project_sites): | |||
f"Parameter {EVALUATION_PERSPECTIVE} has to be either {AC_SYSTEM} or {DC_SYSTEM}, but is {case_definitions[case][EVALUATION_PERSPECTIVE]}" | |||
) | |||
|
|||
if USE_BIDIRECTIONAL_INVERTER_CONSTRAINT not in case_definitions[case]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. Maybe write a pytest for this function that makes sure that your two outcomes (USE_BIDIRECTIONAL_INVERTER_CONSTRAINT
is not and is in settings) are checked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes will look into pytest next
@@ -414,6 +415,12 @@ def get_case_definitions(file, sheet_project_sites): | |||
f"Parameter {EVALUATION_PERSPECTIVE} has to be either {AC_SYSTEM} or {DC_SYSTEM}, but is {case_definitions[case][EVALUATION_PERSPECTIVE]}" | |||
) | |||
|
|||
if USE_BIDIRECTIONAL_INVERTER_CONSTRAINT not in case_definitions[case]: | |||
case_definitions[case][USE_BIDIRECTIONAL_INVERTER_CONSTRAINT] = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Especially because of this, I think you need to use json.update()
. With the pytest you will figure out if there are bugs in your code, even if the code initially passes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What will the json.update() do?
@@ -220,9 +224,9 @@ def build(experiment, case_dict): | |||
# ------------point of coupling (feedin)------------# | |||
if case_dict[PCC_FEEDIN_FIXED_CAPACITY] == None: | |||
pass | |||
# pointofcoupling_feedin = None | |||
pointofcoupling_feedin = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intentional removal of #
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is an intentional removal because I need it later on in the code for the constraint_equate_bidirectional_transformer_capacities function (see line 462-471) in case PCC_FEEDIN_FIXED_CAPACITY is None.
) | ||
|
||
# ------------Link rectifier and inverter capacities constraint------------# | ||
if case_dict[USE_BIDIRECTIONAL_INVERTER_CONSTRAINT] != None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can it be None
? Isnt your default False
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be false indeed
CAP_inverter = 0 | ||
CAP_rectifier = 0 | ||
if case_dict[name_transformer_in] != None and case_dict[name_transformer_out] != None: | ||
if case_dict[name_transformer_in] is False and case_dict[name_transformer_out] is False: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this loop do? Dont forget to use in-line comments (#
), and also add docstrings to your functions (typing `r"""`` will probably aid you in writing them)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alright will add comments for the next PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually I took the idea from one of your comments (#155 (comment)) but I'm not sure in what case case_dict[name_transformer_in] is false and why different capacities need to be added (see code below) since there's just one converter
CAP_inverter += model.InvestmentFlow.invest[bus_transformer_in, transformer_in]
CAP_rectifier += model.InvestmentFlow.invest[bus_transformer_out, transformer_out]
Could you clarify what it represents?
If you need inspiration for the pytests, you can find a lot in https://github.com/rl-institut/multi-vector-simulator/, folder |
Hello @remmyd! Haven you been able to work on the pytests since my last, short review? If you let me know if this PR is still up-to-date I can do a review next week. |
#155 @smartie2076 here's the first draft for the link between inverter rectifier capacity as well as for the transformer station. Let me know what you think when you get the chance