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/excess #555

Merged
merged 37 commits into from
Sep 9, 2020
Merged

Fix/excess #555

merged 37 commits into from
Sep 9, 2020

Conversation

smartie2076
Copy link
Collaborator

@smartie2076 smartie2076 commented Sep 3, 2020

Fix #548

Changes proposed in this pull request:

  • Added Evaluation of excess energy for each of the energy carriers and for the whole system, KPIs:
  • Changed C1.total_demand_each_sector() to C1.total_demand_and_excess_each_sector(), now also evaluating the excess energy flows
  • energyBusses now is defined by: LABEL, ASSET_LIST, ENERGY_VECTOR, all functions using energyBusses have been changed to this nomenclature
  • Energy excess sinks now also have parameter ENERGY_VECTOR
  • Feed-in sinks of the DSOs now are capacity optimized and can actually be used
  • Pytest are updated to above changes
  • Move/rename input data for D0 and D1 test: test_data_for_D0/mvs_config.json, test_data_for_D1/mvs_config.json.

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

  • Use in-line comments to explain your code
  • Write docstrings to your code
  • For new functionalities: Explain in readthedocs
  • Write test(s) for your new patch of code
  • 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.

smartie2076 added 5 commits September 3, 2020 16:29
- Add TOTAL_EXCESS to constants_json_strings.py
- Change C1.total_demand_each_sector to
C1.total_demand_and_excess_each_sector and evaluate demand and excess
simultaneously
@smartie2076 smartie2076 self-assigned this Sep 3, 2020
@smartie2076 smartie2076 added the bug Something isn't working label Sep 3, 2020
@smartie2076
Copy link
Collaborator Author

@SabineHaas I noticed that you are reading from a json file for the D0 and D1 tests, eg:

def dict_values():
    answer = load_json(
        os.path.join(TEST_REPO_PATH, TEST_INPUT_DIRECTORY, "test_data_for_D0.json")
    )

This can be difficult when we actually still change that json file. Would it be possible to read the json that is generated from tests/inputs instead, so that we can better update the json files?

@SabineHaas
Copy link
Collaborator

@SabineHaas I noticed that you are reading from a json file for the D0 and D1 tests, eg:

def dict_values():
    answer = load_json(
        os.path.join(TEST_REPO_PATH, TEST_INPUT_DIRECTORY, "test_data_for_D0.json")
    )

This can be difficult when we actually still change that json file. Would it be possible to read the json that is generated from tests/inputs instead, so that we can better update the json files?

Technically it's possible, but the reason to read from a test json file was to avoid failing tests when the input json is changed. As there we don't test the json format but the definition of oemof components for which the test needs data to work with I'd prefer to not read in the json from the input folder. What do you think?

@Bachibouzouk
Copy link
Collaborator

All json files are (should be) covered by the tests in test_input_folder_parameters.py. The function find_json_input_folders of mvs_eland.utils first lists all input json file and then the test ensure all these files have the right fields (i.e. nothing missing, nor nothing extra not listed in the official extra paremeters list).

To be found as an input folder, one has to contain a mvs_config.json file. Thus test_data_for_D0.json will not be detected unless it is changed to a file within a folder test_data_for_D0/mvs_config.json.

I agree with @SabineHaas that one should keep specific files for the specific tests. I propose to do the change I propose to make sure these files follow the right input file structure via test_input_folder_parameters.py

@smartie2076
Copy link
Collaborator Author

All json files are (should be) covered by
To be found as an input folder, one has to contain a mvs_config.json file. Thus test_data_for_D0.json will not be detected unless it is changed to a file within a folder test_data_for_D0/mvs_config.json.

Ah, okay. In that case we should put the file into that folder, so that it's completeness is tested and can easily be completed.

I agree with @SabineHaas that one should keep specific files for the specific tests. I propose to do the change I propose to make sure these files follow the right input file structure via test_input_folder_parameters.py

I am mostly avoiding using input files, but define the inputs of functions right in the test_*.py files. But sure, D1 accesses too much data to write it out.

I am simply worried that we may need to change the json more majorly, and then we will not know if the json for the D1 tests needs to be as it is specifically, or if it can freely be updated.

I will move the file according to the above.

smartie2076 and others added 13 commits September 8, 2020 12:08
- Add parameter 'asset_group'  to determine tuple that accesses oemof
results
- Add global variables to E1: asset_groups_defined_by_influx,
asset_groups_defined_by_outflux
- Define function that determines which flows need to be evaluated,
replacing the strings "input"/"output" with logical expressions:
E1.get_parameter_to_be_evaluated_from_oemof_results(), add pytest
- Define function that defines the tuple that accesses oemof results:
E1.get_touple_for_oemof_results() add pytest
- Update E1.get_optimal_cap() with flow_tuple
- Update E1.get_flow() with flow_tuple
@smartie2076
Copy link
Collaborator Author

smartie2076 commented Sep 8, 2020

@SabineHaas @Bachibouzouk I now made the json input files for D0 and D1 into mvs_config.json files. However, this also means that those json files can not be as short as possible anymore, because it is checked that eg. economic_data, simulation_settings and fixcosts are included. That data is not necessary for the tests. I hope this is fine.

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@Bachibouzouk Bachibouzouk left a comment

Choose a reason for hiding this comment

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

Running the tests I get an error of the EXECUTE_TESTS_ON=master pytest tests/test_input_folder_parameters.py for the tests/test_data/A0_test_output_folder_name input folder: "/home/pfduc/Documents/RLI/mvs_eland/tests/test_data/A0_test_output_folder_name, the key missing_parameters is included."

I commented on a few improvements of docstrings and asked a few questions. The review was quite long and could have been shortened by merging some changes in dev from an independent PR and rebasing from dev (like the numerous linting changes which scatter the attention of the reviewer). I will do a rebase workshop so you'll acquire this skill :)

btw the error message should read "is not included"

CHANGELOG.md Outdated Show resolved Hide resolved
src/mvs_eland/C0_data_processing.py Outdated Show resolved Hide resolved
src/mvs_eland/C0_data_processing.py Show resolved Hide resolved
src/mvs_eland/D1_model_components.py Outdated Show resolved Hide resolved
src/mvs_eland/E1_process_results.py Outdated Show resolved Hide resolved
src/mvs_eland/E3_indicator_calculation.py Show resolved Hide resolved
src/mvs_eland/E3_indicator_calculation.py Outdated Show resolved Hide resolved
tests/test_C0_data_processing.py Show resolved Hide resolved
tests/test_E1_process_results.py Show resolved Hide resolved
@smartie2076 smartie2076 mentioned this pull request Sep 8, 2020
3 tasks
@smartie2076
Copy link
Collaborator Author

Running the tests I get an error of the EXECUTE_TESTS_ON=master pytest tests/test_input_folder_parameters.py for the tests/test_data/A0_test_output_folder_name input folder: "/home/pfduc/Documents/RLI/mvs_eland/tests/test_data/A0_test_output_folder_name, the key missing_parameters is included."

I do not have that error locally...

I commented on a few improvements of docstrings and asked a few questions. The review was quite long and could have been shortened by merging some changes in dev from an independent PR and rebasing from dev (like the numerous linting changes which scatter the attention of the reviewer). I will do a rebase workshop so you'll acquire this skill :)

Yeah, sorry. As I am on a constant deadline right now I just dont have the patience to work with the video you made right now... I am naming all linting commits accordingly, though, so maybe you can skip them?

It was an issue that should now be fixed with a specific black version that has to be installed.

@smartie2076 smartie2076 merged commit 2f82eee into dev Sep 9, 2020
@smartie2076
Copy link
Collaborator Author

@ursulaelmir please be aware of the issue regarding energy vectors and excess, see above.

@smartie2076 smartie2076 deleted the fix/excess branch September 9, 2020 13:28
@ursulaelmir
Copy link
Collaborator

Hi @smartie2076 , when I have a green field and the MVS is investing in PV, and there is a feed-in tariff of 0.05, the excess energy is still going to the excess sink and not to the feed-in. I thought this issue is solved already. I am using the following input folder.
homer_inputs.zip

@smartie2076
Copy link
Collaborator Author

Hi @smartie2076 , when I have a green field and the MVS is investing in PV, and there is a feed-in tariff of 0.05, the excess energy is still going to the excess sink and not to the feed-in. I thought this issue is solved already. I am using the following input folder.

Hm, it should have been solved... That is weird. Should have written a benchmark test for this...

But I have an idea. Please check the line I commented in this PR - maybe the feed-in tariff is defined as a positive value in define_sink() in C0 and nothing in D1 changes that - making the tariff a price instead. Then you only need to multiply your number by -1 (or change the code in the line I highlighted).

@smartie2076
Copy link
Collaborator Author

@ursulaelmir I could also know if that is the issue if you copy the DSO entries from the energyProduction list from the json file in here.

@ursulaelmir
Copy link
Collaborator

ursulaelmir commented Sep 10, 2020

@ursulaelmir I could also know if that is the issue if you copy the DSO entries from the energyProduction list from the json file in here.

processed or results? and which part exactly? maybe this?

"name": [ [ "Electric grid_consumption_source", "Electricity_pdp_bus" ],

@ursulaelmir
Copy link
Collaborator

Hi @smartie2076 , when I have a green field and the MVS is investing in PV, and there is a feed-in tariff of 0.05, the excess energy is still going to the excess sink and not to the feed-in. I thought this issue is solved already. I am using the following input folder.

Hm, it should have been solved... That is weird. Should have written a benchmark test for this...

But I have an idea. Please check the line I commented in this PR - maybe the feed-in tariff is defined as a positive value in define_sink() in C0 and nothing in D1 changes that - making the tariff a price instead. Then you only need to multiply your number by -1 (or change the code in the line I highlighted).

Do you mean line 322 in C0?
So when I define the feed-in tariff as -0.05 instead of 0.05, the sum of the excess is zero and instead the feed-in sink is used. However, this also changes the optimised capacity for PV, RE share and so on. Should I stick to those results for now (with feed-in tariff as -0.05 ?)

@smartie2076
Copy link
Collaborator Author

@ursulaelmir I could also know if that is the issue if you copy the DSO entries from the energyProduction list from the json file in here.

processed or results? and which part exactly? maybe this?

"name": [ [ "Electric grid_consumption_source", "Electricity_pdp_bus" ],

No, this is not enough but below fix seems to work so take that

@@ -777,6 +807,7 @@ def define_dso_sinks_and_sources(dict_values, dso):
price=dict_values[ENERGY_PROVIDERS][dso][FEEDIN_TARIFF],
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you @ursulaelmir try with a negative value in your input data for the feed-in tariff? It may be that the price is added as cost, not as revenue. @ursulaelmir can you check in D1 as well?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes I did that (using a negative feed-in tariff), I wrote the outcome in my previous comment.

@smartie2076
Copy link
Collaborator Author

Do you mean line 322 in C0?

Ah, sorry - the app works different than the website, so I did not post the comment. Now you should see it. Line 807.

So when I define the feed-in tariff as -0.05 instead of 0.05, the sum of the excess is zero and instead the feed-in sink is used.

Perfect, that is how it should be (as long as there are no investments necessary for the excess generation to reach the feed-in sink, eg. Though an inverter, which is not the case here).

However, this also changes the optimised capacity for PV, RE share and so on. Should I stick to those results for now (with feed-in tariff as -0.05 ?)

That makes sense: the optimization problem is a completely different one, as soon as one variable changes (and geneates revenue at that).

As the system can make revenue with the feed-in it might be beneficial to have a slight excess generation sometimes, if consumption costs can be avoided that way. I assume the PV capacity increases?

Is there something that worries you with the results?

@ursulaelmir
Copy link
Collaborator

Is there something that worries you with the results?

Not at all :) I think now the results are much more comparable (except for the LCO_el and LCO_H2). Thanks!!!!

@smartie2076 smartie2076 mentioned this pull request Sep 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Feedin-sinks are never used, as they are not optimized
4 participants