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

Feature/stratified therm storage a1 #718

Merged
merged 12 commits into from
Jan 29, 2021
Merged

Conversation

MaGering
Copy link
Collaborator

@MaGering MaGering commented Dec 15, 2020

Fix #667

With this PR it is possible to model the stratified thermal storage component, which has been developed in oemof.thermal, passing two further parameter fixed_losses_relative and fixed_losses_absolute to the storage component.

In contrast to PR #711 the two parameters are processed in the mvs via A1_csv_to_json.py and hence are not optional parameters anymore (see #711 (comment) and #711 (comment)).

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.

@smartie2076
Copy link
Collaborator

Hi Marie! I am confused. Can you explain to me why you created a specific branch for this? If it is because you did a main decision, you could have merged this branch into the other branch (#711) for a seperate fix.

@MaGering
Copy link
Collaborator Author

Hi Marie! I am confused. Can you explain to me why you created a specific branch for this? If it is because you did a main decision, you could have merged this branch into the other branch (#711) for a seperate fix.

Oh sorry, I understood this from the following sentence in #711 (comment):

you could also do this in another PR, if you want to keep this one clean

I also thought that it would be useful to leave PR #711 as it is in case I or somebody else should decide to implement the thermal losses through storage_*.csv file without workaround (as soon as storage_*.csv is implemented in the regular mvs workflow, just like energyStorage.csv for example is).

@MaGering MaGering changed the base branch from dev to feature/stratified_therm_storage December 16, 2020 13:45
This was referenced Dec 18, 2020
Copy link
Collaborator

@smartie2076 smartie2076 left a comment

Choose a reason for hiding this comment

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

Hi @MaGering!

So, I would like to keep the MVS so, that it allows that you two new parameters are not defined. I propose a solution. Please try to implement it, and I hope that you can then revert all the input files back to where they where. If not, I can take a look in January.

You can always work on this branch in the next week for your simulations.

What is the result regarding minimum=1?

docs/MVS_parameters_list.csv Outdated Show resolved Hide resolved
input_template/csv_elements/storage_01.csv Outdated Show resolved Hide resolved
src/multi_vector_simulator/A1_csv_to_json.py Show resolved Hide resolved
src/multi_vector_simulator/C0_data_processing.py Outdated Show resolved Hide resolved
tests/inputs/csv_elements/storage_01.csv Outdated Show resolved Hide resolved
tests/test_data/csv_storage_wrong_column_name.csv Outdated Show resolved Hide resolved
tests/test_data/csv_storage_wrong_parameter.csv Outdated Show resolved Hide resolved
tests/test_data/csv_storage_wrong_values.csv Outdated Show resolved Hide resolved
tests/test_A1_csv_to_json.py Outdated Show resolved Hide resolved
@MaGering
Copy link
Collaborator Author

So, I would like to keep the MVS so, that it allows that you two new parameters are not defined. I propose a solution. Please try to implement it, and I hope that you can then revert all the input files back to where they where. If not, I can take a look in January.

You can always work on this branch in the next week for your simulations.

Cool, thank you very much Martha! I will try to get the simulations running with your propositions. :)

What is the result regarding minimum=1?

Well that's a little complicated: It is actually this raise AttributeError(e3) in _check_invest_attributes(...)of oemof.solphs components.py which forces me to set a minimum capacity.

I tested several use cases and discovered, that the simulation would run without error if this AttributeError didn't exist. I would not have to specify a minimum capacity. I still couldn't figure out (from rtd etc.) why it is necessary.

@smartie2076
Copy link
Collaborator

smartie2076 commented Dec 18, 2020

What is the result regarding minimum=1?

Well that's a little complicated: It is actually this raise AttributeError(e3) in _check_invest_attributes(...)of oemof.solphs components.py which forces me to set a minimum capacity.

I tested several use cases and discovered, that the simulation would run without error if this AttributeError didn't exist. I would not have to specify a minimum capacity. I still couldn't figure out (from rtd etc.) why it is necessary.

So it is a generic oemof-solph issue for the new attributes? Weird!

I hate the idea of it but... Have you tried with minimum=0.000000001)? Very dirty hot fix, but it would allow you (on a fork or your own branch) to run your simulations over the winter...

@MaGering
Copy link
Collaborator Author

So it is a generic oemof-solph issue for the new attributes? Weird!

For my use case at least it seems to be. However I can not estimate whether there is not a necessity of it or advantages in other use cases.

I hate the idea of it but... Have you tried with minimum=0.000000001)? Very dirty hot fix, but it would allow you (on a fork) to run your simulations over the winter...

Yes, I have tried this and it works as well. But I don't like this either. I thought maybe to first find the purpose of the AttributeError. If there is a good reason I would go with the dirty hot fix. But otherwise I would propose to just do a warning instead.

An information I got from Jann is, that the absolute losses exist independently from the installed capacity so they would exist even if there was no storage installed. However, this has not been confirmed when I tested it in separate use cases (one without thermal energy storage and one with a super expensive thermal energy storage. The super expensive one hasn't been installed and in both cases the heat demand was the same).

I think I will address this issue after the holidays again.

Copy link
Collaborator

@smartie2076 smartie2076 left a comment

Choose a reason for hiding this comment

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

Nice that it works. Will look at it in greater detail after the holidays.

Please create the pytest then to make sure this works (pytest for changed function in A1 + benchmark test).

src/multi_vector_simulator/A1_csv_to_json.py Show resolved Hide resolved
@mahendrark mahendrark force-pushed the feature/stratified_therm_storage branch from 6998795 to 8a5e528 Compare December 22, 2020 18:37
@smartie2076
Copy link
Collaborator

Hi @MaGering! So, should I review this PR now?

@MaGering
Copy link
Collaborator Author

MaGering commented Jan 5, 2021

Hi @MaGering! So, should I review this PR now?

Hello Martha,

I found that I've been wrong with the losses. They are comparatively small but they are being calculated as Jann said even if no storage is installed (because too expensive). So the investment.minimum parameter must be higher than the losses. Therefore setting it to 0.00000001 doesn't make much sense. In this case we could skip as well the e3 RaiseAttributeError, which is forcing us to choose investment.minimum.

Before merging this branch the question remains how we want to proceed with this to avoid hardcoding an investment minimum we actually don't want. An idea, @c-moeller had, is integrating investment.minimum instorage_*.csv with a default value of 0. Only if the stratified thermal storage is used, the user should set it to a value higher than the losses.

Can you estimate the effort of doing this?

@MaGering
Copy link
Collaborator Author

MaGering commented Jan 5, 2021

I am also trying to understand the calculations in oemof.solph and see if there is another way to take out the losses and avoid the e3 RaiseAttributeError.

@MaGering MaGering changed the base branch from feature/stratified_therm_storage to dev January 5, 2021 15:08
@smartie2076
Copy link
Collaborator

Hi @MaGering ! Could you maybe summarize the findings of our call yesterday here / your plans so that nothing gets lost?

@MaGering
Copy link
Collaborator Author

MaGering commented Jan 7, 2021

Hi @MaGering ! Could you maybe summarize the findings of our call yesterday here / your plans so that nothing gets lost?

Sure! :)

Since the problem of the investment.minimum parameter equal to zero and at the same time existing thermal losses cannot be solved by me at the moment, @smartie2076 and I decided to work with the investment.minimum parameter. @smartie2076 had the idea to hardcode the parameter in D1_model_components.py to zero. Only in case of a stratified heat storage (fixed_thermal_losses_absolute or fixed_thermal_losses_relative exist) it will be (also there) set to 1.

I wrote an issue in oemof.thermal describing the problem.

Now there are the following open tasks:

  • Implement investment.minimum in D1_model_components.py
  • Explain this dirty fix in readthedocs
  • Write tests to check the the functionalities of investment.minimum

@smartie2076 smartie2076 marked this pull request as draft January 7, 2021 15:56
@smartie2076
Copy link
Collaborator

Do you also plan to integrade a benchmark test with the stratified termal storage in tests/inputs/benchmark_test_inputs? I think that would be necessary, as it is essential another component. You could also think to compare this to the result of a thermal storage which is modelled without your losses.

...I am not sure how we can make sure that the generic storage in dev and in your PR has identical results, but to do it manually. You did that again, right?

@MaGering
Copy link
Collaborator Author

MaGering commented Jan 7, 2021

Do you also plan to integrade a benchmark test with the stratified termal storage in tests/inputs/benchmark_test_inputs? I think that would be necessary, as it is essential another component. You could also think to compare this to the result of a thermal storage which is modelled without your losses.

...I am not sure how we can make sure that the generic storage in dev and in your PR has identical results, but to do it manually. You did that again, right?

Yes, this is what I'm also working on right now besides the pytest of the changed function in A1, documenting the dirty fix in the readthedocs (bullet point from my comment see above) and documenting the changes in CHANGELOG.md.

As for the benchmark test in both cases it is the generic storage component. In case of a stratified thermal storage only the two further thermal losses are passed with the generic storage component. So a thermal storage without the two thermal losses is just a generic storage. :) I think this would be rather a test in oemof.solph but I could try to write a benchmark test where I build a generic storage as it is now in dev and one with thermal losses that equal zero and compare their results. What do you think @smartie2076?
Than I'll write a test which checks if the invested storage capacity is higher with thermal losses passing 1. a numeric (float), 2. a time series. And another test that checks if the invested storage capacity of a storage with no fixed thermal losses passed and fixed thermal losses that equal zero matches. One further test would be to check if the invested storage capacity match when I pass 1. installedCap = 0 and 2. installedCap = NaN.
Is there a test still missing?

So the benchmark tests to implement are:

  • Compare results of fix generic storage as it is now in dev and one with thermal losses that equal zero
  • Compare results of optimize generic storage as it is now in dev and one with thermal losses that equal zero
  • Invested storage capacity is higher with thermal losses passing a numeric (float)
  • Invested storage capacity is higher with thermal losses passing a time series
  • Invested storage capacity of a storage with no fixed thermal losses and fixed thermal losses that equal zero match
  • Invested storage capacity of installedCap = 0 and installedCap = NaN match

@MaGering MaGering marked this pull request as ready for review January 11, 2021 08:58
Copy link
Collaborator

@smartie2076 smartie2076 left a comment

Choose a reason for hiding this comment

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

@MaGering I did not work though the whole PR jet (benchmark test missing). I still need to execute your code locally to make sure everything works.
Some remarks already now, including some RTD stuff. Please add the name of the pytests of functions to the original functions` "Notes" section, and also write debug messages.

@Bachibouzouk with this PR, there are two parameters THERM_LOSSES_REL and THERM_LOSSES_ABS added to the storage_xx.csv. They are not to be used for the MVS within E-Land, and should also be set with a default value if we do use json input files. Therefore, they should also be added to #456 and/or #750.

@SabineHaas I did a pre-check of this PR now. If you had time, it would be great if you also checked it completely. The D1 tests are hard for me to read, so please check those.

CHANGELOG.md Outdated Show resolved Hide resolved
@@ -63,6 +63,61 @@ In case of excessive excess energy, a warning is given that it seems to be cheap
High excess energy can for example result into an optimized inverter capacity that is smaller than the peak generation of installed PV.
This becomes unrealistic when the excess is very high.

Stratified thermal storage
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you change this to Energy storage and descibe how the generic energy storage is used, and then stratified thermal storage as a specific case of energy storages?

Outline of content:
Generic storages are defined with file x and y, and have subassets a, b, c. Self-discharge... efficiency of charge/discharge non-dependent on soc... rate at which battery can be (dis)charged relative to its capacity (crate)... battery lifetime based on years, not charge cycles... for now now KPI measuring the storage utilization (charge cycles, average charge ect).
Stratified storage uses two optional parameters: d, e. If those two are not included in the csv file (or 0), then a normal generic storage is simulated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Generic storages are defined with file x and y, and have subassets a, b, c. Self-discharge... efficiency of charge/discharge non-dependent on soc... rate at which battery can be (dis)charged relative to its capacity (crate)... battery lifetime based on years, not charge cycles... for now now KPI measuring the storage utilization (charge cycles, average charge ect).

I don't quite understand. Isn't this section described in Parameters and Definitions in CSVs/JSON in the docs? I can list the subassets but do I need to describe them again or can I just reference to Parameters and Definitions in CSVs/JSON? And should I actually add the fixed thermal losses to MVS_parameters.rst as well or did you not want them in there?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is there a reason why you don't use references to other rst files in rtd, or did you just not have a need for it until now?

Otherwise I would suggest this:

  • Adding sphinx.ext.autosectionlabel to the extensions in docs/config.py
  • Adding a label to the subsection storage_*.csv in docs/MVS_parameters.rst
  • Adding the following sentence to docs/Model_Assumptions.rst:
    Generic storages are defined with file `energyStorage.csv` and `storage_*.csv` and have subassets, which are listed in :ref:`storage_*.csv`.

I've tested it locally and it works.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So I've found a solution in commit ad4ef15

What do you think @smartie2076? Is it necessary to describe the generic storage in more detail?

docs/Model_Assumptions.rst Show resolved Hide resolved
docs/Model_Assumptions.rst Outdated Show resolved Hide resolved
tests/inputs/mvs_config.json Outdated Show resolved Hide resolved
tests/test_A1_csv_to_json.py Outdated Show resolved Hide resolved
tests/test_A1_csv_to_json.py Show resolved Hide resolved
@SabineHaas
Copy link
Collaborator

@SabineHaas I did a pre-check of this PR now. If you had time, it would be great if you also checked it completely. The D1 tests are hard for me to read, so please check those.

Will do that :) @MaGering could you give me a hint after you have worked on all the comments? I think it makes sense that I wait until then.

@MaGering
Copy link
Collaborator Author

Will do that :) @MaGering could you give me a hint after you have worked on all the comments? I think it makes sense that I wait until then.

Sure, this makes sense! :)

Copy link
Collaborator

@smartie2076 smartie2076 left a comment

Choose a reason for hiding this comment

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

Really impressive work, especially the benchmark tests :D
You can add a mention of your benchmark tests here, summarized in one bullet point.

I reviewed the PR again, ran all tests locally and think that it can be merged. There is only the one issue that if you only change the tests/input/mvs_config.json file, not the csvs, it is very likely that those will be overwritten in future PR.

There are also some warning messages that you could adress by changing the input file values of your input files of installedCap from NaN to a valid value (Warning for multiple of your input files):

In file storage_optimize_without_fixed_thermal_losses.csv the parameter installedCap in column storage capacity is NaN. Please insert a value of 0 or int. For this simulation
the value is set to 0 automatically.

Next time with such a big PR you should clean it up afterwards (do git rebase --soft commitnumber and redo your commits), as it is much easier to read. Honestly, I would probably have been quicker about it then as it just requires less time and concentration that way ;)

@@ -37,6 +38,10 @@ Here is a template for new release sections
- Gather all missing MVS parameters and raise a single error listing all of them (#761)
- Add `set_default_values` argument to the `B0.load_json` function to set default values of missing parameter which is listed in `KNOWN_EXTRA_PARAMETERS`(#761)
- Add `flag_missing_values` argument to the `B0_load_json` function to allow switching between `MissingParameterWarning` and `MissingParameterError`(#761)
- In `test_A1_csv_to_json.py` tests were added that check whether default values of `0` are set for `fixed_losses_relative` and `fixed_losses_absolute` in case the user does not pass these two parameters (#718)
- In `test_D1_model_components.py` tests were added that check whether the `GenericStorage` parameter `investment.minimum` is set to `0` in case `fixed_losses_relative` and `fixed_losses_absolute` are not passed and to `1` in case they are passed as times series or floats. At this time it is not possible to do an ivestment optimization of a stratified thermal energy storage without a non-zero `investment.minimum` (see this [issue](https://github.com/oemof/oemof-thermal/issues/174)) (#718)
- The two optional parameters `fixed_losses_relative` and `fixed_losses_absolute` were added in `tests/inputs/mvs_config.json` (#718)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, we will again and again recompile mvs_config.json (from the tests/inputs csv data), so adding the parameters here will probably be lost. Is there a reason that you have this additionally to the benchmark test input files?

docs/Model_Assumptions.rst Outdated Show resolved Hide resolved
src/multi_vector_simulator/A1_csv_to_json.py Outdated Show resolved Hide resolved
src/multi_vector_simulator/A1_csv_to_json.py Show resolved Hide resolved
tests/inputs/mvs_config.json Outdated Show resolved Hide resolved
"csv_elements",
)

json = A1.create_json_from_csv(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Very efficient :D

@smartie2076 smartie2076 self-requested a review January 27, 2021 12:03
Copy link
Collaborator

@smartie2076 smartie2076 left a comment

Choose a reason for hiding this comment

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

Aha, wait a second. @Bachibouzouk @MaGering introduces two parameters here that are not possible to add to KNOWS_EXTRA_PARAMETERS (yet). They are added in A1. However, if EPA provides us with input without those parameters, the simulation will fail. I tested that by running the mvs with the json input file without the two parameters:

13:03:01-INFO-Initializing oemof simulation.
13:03:01-INFO-Adding components to oemof energy system model...
Traceback (most recent call last):
  File "mvs_tool.py", line 4, in <module>
    main()
  File "d:\pycharmprojects\mvs_eland\src\multi_vector_simulator\cli.py", line 160, in main
    dict_values, save_energy_system_graph=save_energy_system_graph
  File "d:\pycharmprojects\mvs_eland\src\multi_vector_simulator\D0_modelling_and_optimization.py", line 91, in run_oemof
    dict_values, dict_model, model
  File "d:\pycharmprojects\mvs_eland\src\multi_vector_simulator\D0_modelling_and_optimization.py", line 195, in adding_assets_to_energysystem_model
    model, dict_values[asset_group][asset], **dict_model
  File "d:\pycharmprojects\mvs_eland\src\multi_vector_simulator\D1_model_components.py", line 153, in storage
    **kwargs,
  File "d:\pycharmprojects\mvs_eland\src\multi_vector_simulator\D1_model_components.py", line 328, in check_optimize_cap
    func_optimize(model, dict_asset, **kwargs)
  File "d:\pycharmprojects\mvs_eland\src\multi_vector_simulator\D1_model_components.py", line 629, in storage_optimize
    float(dict_asset[STORAGE_CAPACITY][losses][VALUE])
KeyError: 'fixed_thermal_losses_relative'

I am not sure if we need to make it so, that the MVS always adds the parameters after reading the json file, or if this is simlpy something that has to be added via the parser.

Remark @MaGering: Actually, adding the parameters to the json file is correct, sorry! After all, the mvs_config.json file would be generated from the csv files with those two parameters. The issue is only with the EPA.

@Bachibouzouk
Copy link
Collaborator

Aha, wait a second. @Bachibouzouk @MaGering introduces two parameters here that are not possible to add to KNOWS_EXTRA_PARAMETERS (yet). They are added in A1. However, if EPA provides us with input without those parameters, the simulation will fail. I tested that by running the mvs with the json input file without the two parameters:

@MaGering - you can add the two parameters to the dict_values with their default values. dict_asset there contains the storage asset parameters. In this place of the code we are within a for loop over the asset parameters (the keys of the dict of the storage asset which is under "energyStorage") you can use an if statement insert your new parameters

for example, say your extra parameter is within ENERGY_STORAGE, storage01, INPUT_POWER

if asset_label == INPUT_POWER:
    if myparameterlabel not in dict_asset[asset_label][INPUT_POWER]:
        dict_asset[asset_label][INPUT_POWER][myparameterlabel] = myparametervalue

@MaGering
Copy link
Collaborator Author

Aha, wait a second. @Bachibouzouk @MaGering introduces two parameters here that are not possible to add to KNOWS_EXTRA_PARAMETERS (yet). They are added in A1. However, if EPA provides us with input without those parameters, the simulation will fail. I tested that by running the mvs with the json input file without the two parameters:

@MaGering - you can add the two parameters to the dict_values with their default values. dict_asset there contains the storage asset parameters. In this place of the code we are within a for loop over the asset parameters (the keys of the dict of the storage asset which is under "energyStorage") you can use an if statement insert your new parameters

for example, say your extra parameter is within ENERGY_STORAGE, storage01, INPUT_POWER

if asset_label == INPUT_POWER:
    if myparameterlabel not in dict_asset[asset_label][INPUT_POWER]:
        dict_asset[asset_label][INPUT_POWER][myparameterlabel] = myparametervalue

Hi @Bachibouzouk! Thanks for helping me out with this. Can you give me a hint how I can test my adaptations? How can I go to data_parser.py in debug mode for example? Running the simulations I don't get there.

@MaGering
Copy link
Collaborator Author

Really impressive work, especially the benchmark tests :D
You can add a mention of your benchmark tests here, summarized in one bullet point.

I've done this in commit f827132. Both links should work as soon as this PR is merged to dev. If you click them now it will lead you to a 404.

I reviewed the PR again, ran all tests locally and think that it can be merged. There is only the one issue that if you only change the tests/input/mvs_config.json file, not the csvs, it is very likely that those will be overwritten in future PR.

Just to be sure, I don't have to do anything with tests/input/mvs_config.json anymore, right @smartie2076?

There are also some warning messages that you could adress by changing the input file values of your input files of installedCap from NaN to a valid value (Warning for multiple of your input files):

In file storage_optimize_without_fixed_thermal_losses.csv the parameter installedCap in column storage capacity is NaN. Please insert a value of 0 or int. For this simulation
the value is set to 0 automatically.

You're absolutely right! I changed that with commit 5761ce2. Now there is only one of these warnings in test_installedCap_zero_equal_installedCap_nan(self, margs) due to testing an installedCap of 0 vs. NA.

Next time with such a big PR you should clean it up afterwards (do git rebase --soft commitnumber and redo your commits), as it is much easier to read. Honestly, I would probably have been quicker about it then as it just requires less time and concentration that way ;)

I'm sorry. Next time I will learn doing this! Thanks for your patience reviewing this PR and your credit above. :)

@MaGering
Copy link
Collaborator Author

Aha, wait a second. @Bachibouzouk @MaGering introduces two parameters here that are not possible to add to KNOWS_EXTRA_PARAMETERS (yet). They are added in A1. However, if EPA provides us with input without those parameters, the simulation will fail. I tested that by running the mvs with the json input file without the two parameters:

@MaGering - you can add the two parameters to the dict_values with their default values. dict_asset there contains the storage asset parameters. In this place of the code we are within a for loop over the asset parameters (the keys of the dict of the storage asset which is under "energyStorage") you can use an if statement insert your new parameters
for example, say your extra parameter is within ENERGY_STORAGE, storage01, INPUT_POWER

if asset_label == INPUT_POWER:
    if myparameterlabel not in dict_asset[asset_label][INPUT_POWER]:
        dict_asset[asset_label][INPUT_POWER][myparameterlabel] = myparametervalue

Hi @Bachibouzouk! Thanks for helping me out with this. Can you give me a hint how I can test my adaptations? How can I go to data_parser.py in debug mode for example? Running the simulations I don't get there.

@Bachibouzouk I provided the adjustment in data_parser.py with commit 98ec9dc. Is this going to work or do I need to change something?

@MaGering
Copy link
Collaborator Author

Because of issue #780 I had to add minimal_degree_of_autonomy in constraints.csv with commit e2e3a91.

@Bachibouzouk
Copy link
Collaborator

Bachibouzouk commented Jan 28, 2021

Hi @Bachibouzouk! Thanks for helping me out with this. Can you give me a hint how I can test my adaptations? How can I go to data_parser.py in debug mode for example? Running the simulations I don't get there.

@MaGering

When #781 is merged you can then run

EXECUTE_TESTS_ON=master pytest tests/test_benchmark_scenarios.py::test_benchmark_EPA_run_through

you can also run

    with open(os.path.join("tests","benchmark_test_inputs" "epa_benchmark.json")) as json_file:
        epa_dict = json.load(json_file)

    dict_values = convert_epa_params_to_mvs(epa_dict)

in a standalone .py file

@Bachibouzouk
Copy link
Collaborator

Bachibouzouk commented Jan 28, 2021

Next time with such a big PR you should clean it up afterwards (do git rebase --soft commitnumber and redo your commits)

does that work @smartie2076. I would do git rebase dev -i to rebase on dev (only the changes not in dev will be displayed then) first, then git reset --soft commitnumber (this destroys the commit but keep the changes). Then redo nice commits. @MaGering if you want we can do this together (if you want to learn), don't worry, we'll do a copy of the branch, we will not loose any of your work :)

@Bachibouzouk
Copy link
Collaborator

@Bachibouzouk I provided the adjustment in data_parser.py with commit 98ec9dc. Is this going to work or do I need to change something?

This should work. Aren't your parameter defined as {UNIT: "factor", VALUE: 0}?

@MaGering
Copy link
Collaborator Author

@Bachibouzouk I provided the adjustment in data_parser.py with commit 98ec9dc. Is this going to work or do I need to change something?

This should work. Aren't your parameter defined as {UNIT: "factor", VALUE: 0}?

Yes! For the relative losses... For absolute losses it should be {UNIT: "kWh", VALUE: 0}.
So, do I need to do the following instead?

if parameter == THERM_LOSSES_REL:
      dict_asset[asset_label][STORAGE_CAPACITY][parameter] = {UNIT: "factor", VALUE: 0}
elif parameter == THERM_LOSSES_ABS:
      dict_asset[asset_label][STORAGE_CAPACITY][parameter] = {UNIT: "kWh", VALUE: 0}

Without as usual going trial and error I couldn't find out what is needed in this line... 😅

@Bachibouzouk
Copy link
Collaborator

So, do I need to do the following instead?

Yes, one could then question the need of a for loop when we have only two parameters with different treatment (I think it takes less lines of code to not have the loop ... ;))

@MaGering
Copy link
Collaborator Author

So, do I need to do the following instead?

Yes, one could then question the need of a for loop when we have only two parameters with different treatment (I think it takes less lines of code to not have the loop ... ;))

Yes you're right. It's late. ;) I'll kick the loop when I correct it.

@MaGering MaGering force-pushed the feature/stratified_therm_storage_A1 branch from 31e268b to c7c36bd Compare January 29, 2021 17:41
@MaGering MaGering merged commit ff529b5 into dev Jan 29, 2021
@MaGering MaGering deleted the feature/stratified_therm_storage_A1 branch January 29, 2021 17:52
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.

Integrate stratified thermal storage of oemof.thermal
4 participants