Feature/stratified therm storage #711

MaGering · 2020-12-11T12:20:20Z

Fix Issue #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.

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

…ge functions

smartie2076

Looking good so far - but is this working now? Can you post a description of the current status?

Also, please update the RTD for this functionality. Meaning:

Add relative_thermal_losses and absolute_thermal_losses into this csv so that it is explained here.
Add a sub-section on how to simulate a thermal energy storage with an advanced level of detail here. Also mention possible pre-processing functions that can be used from oemof.thermal. If possible, also describe how normal storages would be modelled ;)
Add the parameters as optional parameters below this section head
Probably you need to adapt some pytest of D1 (see tests/test_D1_*py)
Post the simulation results (pdf) of a scenario calculated with the dev branch and one with your branch and the new storage (but rel_losses/abs_losses = 0) to this PR, so that I can check online if strange things happen.
Create a new benchmark test in (this folder)[https://github.com/rl-institut/multi-vector-simulator/tree/dev/tests/benchmark_test_inputs] (tests/benchmark_tests_inputs) for your feature by copying one of the other tests, and writing the pytest (in this file)[https://github.com/rl-institut/multi-vector-simulator/blob/dev/tests/test_benchmark_scenarios.py]. We can discuss possible assertions together, which would validate that your asset works as planned.
Consider whether you need additonal KPI for the storage now, eg. to measure the thermal losses of your asset.

src/multi_vector_simulator/utils/constants_json_strings.py

src/multi_vector_simulator/utils/constants.py

Co-authored-by: smartie2076 <[email protected]>

…_Assumptions.rst

MaGering · 2020-12-12T13:53:45Z

I also need to adapt these tests:

test_D0_modelling_and_optimization.py
test_benchmark_KPI.py
test_E0_evaluation.py
test_E1_process_results.py

MaGering · 2020-12-12T16:46:42Z

I implemented the two parameters in the mvs but have the following problems with it, when testing them with the simulation in pvcompare on this branch:

They are not optional. If I don't pass them I get an error message and the simulation stops
It seems like they are not processed. I get a warning message no matter if I pass a float or a time series:

Also I adapted the test so they would run but I'm not sure if they make much sense. I set the losses in the benchmark test Economic_KPI_C2_E2 and inputs_for_E1 to zero since it is an electrical storage. However, this is related to the problem described above (1.). It would make more sense to not pass them at all for an electrical storage.

Surely there need to be a separate benchmark test as soon as the parameters work as intended. 🙃

@smartie2076 Could you help me solving the two above mentioned problems? Please tell me if you need further information!

smartie2076 · 2020-12-12T19:05:48Z

Hi @MaGering!

Ah, that is a pity. I already have an idea what's happening, but I need more info. It would be great if you could attach the complete log file (it tells me until when everything is okay). Please use the logging depth DEBUG then.

Can you copy the five warnings into the post as text, so that I can quote them individually?

MaGering · 2020-12-13T13:49:10Z

Hi @MaGering!

Ah, that is a pity. I already have an idea what's happening, but I need more info. It would be great if you could attach the complete log file (it tells me until when everything is okay). Please use the logging depth DEBUG then.

Can you copy the five warnings into the post as text, so that I can quote them individually?

Thanks for your quick reply!

Here's the mvs_logfile.log
I set display_output="debug" for DEBUG logging depth. Is this correct?

Here are the warnings:

/home/local/RL-INSTITUT/marie-claire.gering/Repositories/multi-vector-simulator/src/multi_vector_simulator/A1_csv_to_json.py:556: MissingParameterWarning: You are not using the parameter fixed_thermal_losses_relative for asset group energyStorage, which allows considering relative thermal energy losses (Values: Float). This is an advanced setting that most users can ignore.. This parameter is set to it's default value 0, which can influence the results.In the next release, this parameter will required.
  + "In the next release, this parameter will required."
/home/local/RL-INSTITUT/marie-claire.gering/Repositories/multi-vector-simulator/src/multi_vector_simulator/A1_csv_to_json.py:556: MissingParameterWarning: You are not using the parameter fixed_thermal_losses_absolute for asset group energyStorage, which allows considering relative thermal energy losses (Values: Float). This is an advanced setting that most users can ignore.. This parameter is set to it's default value 0, which can influence the results.In the next release, this parameter will required.
  + "In the next release, this parameter will required."
/home/local/RL-INSTITUT/marie-claire.gering/Repositories/multi-vector-simulator/src/multi_vector_simulator/A1_csv_to_json.py:345: WrongParameterWarning: The storage parameter fixed_thermal_losses_relative of the file /home/local/RL-INSTITUT/marie-claire.gering/Repositories/pvcompare/pvcompare/data/mvs_inputs_template_sector_coupling/csv_elements/storage_02.csv is not recognized. It will not be considered in the simulation.
  f"The storage parameter {i} of the file "
/home/local/RL-INSTITUT/marie-claire.gering/Repositories/multi-vector-simulator/src/multi_vector_simulator/A1_csv_to_json.py:345: WrongParameterWarning: The storage parameter fixed_thermal_losses_absolute of the file /home/local/RL-INSTITUT/marie-claire.gering/Repositories/pvcompare/pvcompare/data/mvs_inputs_template_sector_coupling/csv_elements/storage_02.csv is not recognized. It will not be considered in the simulation.
  f"The storage parameter {i} of the file "

smartie2076 · 2020-12-14T08:52:41Z

Here's the mvs_logfile.log
I set display_output="debug" for DEBUG logging depth. Is this correct?

Okay, looking good otherwise. The simulation does not terminate as far as I can see though?

Well, no wonder, because actually I saw that there is one thing missing for what you what to do in our code. I did not consider that before.

So, these warnings tell you that you did not define fixed_thermal_losses_relative and fixed_thermal_losses_absolute in your energyStorage.csv (!). You defined that it is needed here and here. The error message is correct - but actually, this is also not what you wanted it to do!

/home/local/RL-INSTITUT/marie-claire.gering/Repositories/multi-vector-simulator/src/multi_vector_simulator/A1_csv_to_json.py:556: MissingParameterWarning: You are not using the parameter fixed_thermal_losses_relative for asset group energyStorage, which allows considering relative thermal energy losses (Values: Float). This is an advanced setting that most users can ignore.. This parameter is set to it's default value 0, which can influence the results.In the next release, this parameter will required.
/home/local/RL-INSTITUT/marie-claire.gering/Repositories/multi-vector-simulator/src/multi_vector_simulator/A1_csv_to_json.py:556: MissingParameterWarning: You are not using the parameter fixed_thermal_losses_absolute for asset group energyStorage, which allows considering relative thermal energy losses (Values: Float). This is an advanced setting that most users can ignore.. This parameter is set to it's default value 0, which can influence the results.In the next release, this parameter will required.

What we actually should do is require it for storage_*.csv (not ENERGY_STORAGE). As we did not do that, you get a warning, as the MVS is confused why you added the two new parameters to the file:

/home/local/RL-INSTITUT/marie-claire.gering/Repositories/multi-vector-simulator/src/multi_vector_simulator/A1_csv_to_json.py:345: WrongParameterWarning: The storage parameter fixed_thermal_losses_relative of the file /home/local/RL-INSTITUT/marie-claire.gering/Repositories/pvcompare/pvcompare/data/mvs_inputs_template_sector_coupling/csv_elements/storage_02.csv is not recognized. It will not be considered in the simulation.
/home/local/RL-INSTITUT/marie-claire.gering/Repositories/multi-vector-simulator/src/multi_vector_simulator/A1_csv_to_json.py:345: WrongParameterWarning: The storage parameter fixed_thermal_losses_absolute of the file /home/local/RL-INSTITUT/marie-claire.gering/Repositories/pvcompare/pvcompare/data/mvs_inputs_template_sector_coupling/csv_elements/storage_02.csv is not recognized. It will not be considered in the simulation.

Usually, the parameters that are required are mentioned here, but when you check this out, you will actually notice that we do not have required parameters for storage_*.csv. This is because we do not need to have a storage_*.csv in our input files, so we also did not check those parameters.

Now, the MVS is confused about your inputs!

Sadly for that file we do not have the neat parameter check as for the other files, but the checks are hidden somewhere in A1, maybe here. What can you do about this now?

Either improve the general MVS by adding a parameter check for storage_*.csv just like for the other files (in another PR), which would have the benefit that you could still use the warning definitions in constants.py.
Or tweaking around and adding exceptions to A1 with the benefit that you can add a logging.debug message instead of a warning for the additional storage parameters (you could also do this in another PR, if you want to keep this one clean)

smartie2076 · 2020-12-14T09:19:52Z

Ah, yeah, it is defined where I thought and you could add the new parameters here. Then, those are required parameters, though!

MaGering · 2020-12-15T10:07:56Z

Thank you for looking at this!

Either improve the general MVS by adding a parameter check for storage_*.csv just like for the other files (in another PR), which would have the benefit that you could still use the warning definitions in constants.py.

Or tweaking around and adding exceptions to A1 with the benefit that you can add a logging.debug message instead of a warning for the additional storage parameters (you could also do this in another PR, if you want to keep this one clean)

I'm afraid I have to rule out the first option for now due to time constraints. That's why I would stick to the procedure described in your second option and do the changes on a new branch.

smartie2076

Some comments, may already be adressed in #718

smartie2076 · 2020-12-18T11:35:33Z

docs/Model_Assumptions.rst

+Stratified thermal storage
+##########################
+
+In order to model a stratified thermal energy storage the two parameters `fixed_losses_relative` and `fixed_losses_absolute` are passed through `storage_xx.csv`.


Suggested change

In order to model a stratified thermal energy storage the two parameters `fixed_losses_relative` and `fixed_losses_absolute` are passed through `storage_xx.csv`.

In order to model a stratified thermal energy storage the two parameters `fixed_losses_relative` and `fixed_losses_absolute` are passed to `storage_xx.csv`.

You do not have to have linebreaks for each sentence, but you can.

Please add something that explicitly states that the parameters are not required for storage_*.csv.

smartie2076 · 2020-12-18T11:38:04Z

docs/Model_Assumptions.rst

+
+Please see the `oemof.thermal` `examples <https://github.com/oemof/oemof-thermal/tree/dev/examples/stratified_thermal_storage>`__ and the `documentation  <https://oemof-thermal.readthedocs.io/en/latest/stratified_thermal_storage.html>`__ for further information.
+
+For an investment optimization the height of the storage should be left open in the precalculations and `installedCap` should be set to 0 or NaN.


Suggested change

For an investment optimization the height of the storage should be left open in the precalculations and `installedCap` should be set to 0 or NaN.

For an investment optimization the height of the storage should be left open in the precalculations and `installedCap` should be set to 0 or `NaN`.

Can installedCap be NaN? It should always be a number, as far as I know. Please test this.

Can installedCap be NaN? It should always be a number, as far as I know. Please test this.

Actually I have. I was also wondering how this works, but it does. The background is, that I implemented precalculations in pvcompare, which calculate the oemof.thermal parameter nominal_storage_capacity (= mvs parameter installedCap = oemof.solph parameter Investment(existing)). If you want to do an investment optimization you leave the height open which leads to nominal_storage_capacity= NaN in the precalculations. If you pass the height as a limiting parameter (no investment optimization) nominal_storage_capacity is a fix number in the precalculations. In the precalculations the mvs parameter installedCap is overwritten with the corresponding value.

And this is why I documented it in this way. Does it make sense?

src/multi_vector_simulator/utils/constants.py

smartie2076 · 2020-12-18T11:41:01Z

tests/benchmark_test_inputs/Economic_KPI_C2_E2/csv_elements/storage_1.csv

+fixed_thermal_losses_relative,factor,0,,
+fixed_thermal_losses_absolute,kWh,0,,


I hope that we can keep fixed_thermal_losses_relative and fixed_thermal_losses_absolute optional parameters. If so, please remove the parameters from all other input files, and only leave one benchmark tests for your own feature.

tests/inputs/mvs_config.json

tests/test_data/inputs_for_D0/mvs_config.json

tests/test_data/inputs_for_D1/mvs_config.json

tests/test_data/inputs_for_E1/csv_elements/storage_1.csv

MaGering · 2020-12-22T17:29:59Z

Somehow I messed up this branch @smartie2076 :-(
I accidentally merged changes made in PR #718.

Can I do the following to fix that?

git reset --hard 8a5e528b48238c7a272ba72b6a9873934be71f03
git push --force origin feature/stratified_therm_storage
git merge dev

I've never forced a push but I can't find another way. Will all the comments from PR #718 stay either way? Can I do something else to go back to where it was?
I'm sorry for that mess.

smartie2076 · 2020-12-22T17:39:07Z

@mahendrark can you help @MaGering with this?

Otherwise we will solve it in January.

mahendrark · 2020-12-22T17:47:49Z

@MaGering and @smartie2076

sure, I can take a look at it

@MaGering can you tell me which commit would you like to go back to and do you want to keep the changes made after that commit..

If you can tell me that, I will force push and rebase the branch such that you wouldnt have any merge conflicts and you can take over from there

MaGering · 2020-12-22T18:23:21Z

@MaGering and @smartie2076

sure, I can take a look at it

@MaGering can you tell me which commit would you like to go back to and do you want to keep the changes made after that commit..

If you can tell me that, I will force push and rebase the branch such that you wouldnt have any merge conflicts and you can take over from there

Thank you very much @mahendrark!

I would like to go back to commit 8a5e528 and also to erase the conversations which were merged from this PR #718. It concerns these conversations: #711 (review)

The background is that I branched from feature/stratified_therm_storage (this branch) creating feature/stratified_therm_storage_A1 and made changes there, which were not supposed to get here.

smartie2076 · 2021-01-05T11:08:36Z

Hi @MaGering should I do anything with this PR, is it a backup or is it out-of-date and to be deleted?

MaGering · 2021-01-05T18:31:39Z

Hi @MaGering should I do anything with this PR, is it a backup or is it out-of-date and to be deleted?

Hi Martha,

I will close this PR now. It was a backup but isn't needed anymore since we found a good work around in PR #718. I changed the base in PR #718 to dev.

MaGering added 3 commits December 11, 2020 11:05

Add relative and absolute losses to constants_json_strings.py

9607cc0

Add relative and absolute losses as extra parameter to constants.py

c985f19

Add relative and absolute thermal losses as input parameters of stora…

41a07d9

…ge functions

smartie2076 reviewed Dec 11, 2020

View reviewed changes

src/multi_vector_simulator/utils/constants_json_strings.py Outdated Show resolved Hide resolved

src/multi_vector_simulator/utils/constants.py Outdated Show resolved Hide resolved

src/multi_vector_simulator/utils/constants.py Outdated Show resolved Hide resolved

MaGering and others added 6 commits December 11, 2020 16:18

Apply suggestions from code review

0080035

Co-authored-by: smartie2076 <[email protected]>

Update parameter names to more user friendly naming

69c2da7

Add losses parameter to MVS_parameters_list.csv

257adcb

Add description on how to model a stratified thermal storage in Model…

1329ac1

…_Assumptions.rst

Update tests for D1 to thermal losses of storage

669faa4

Apply black to test_D1_model_components.py

4df13dc

Fix tests

d941b02

MaGering mentioned this pull request Dec 15, 2020

Feature/stratified therm storage a1 #718

Merged

9 tasks

Merge branch 'dev' into feature/stratified_therm_storage

8a5e528

smartie2076 mentioned this pull request Dec 18, 2020

Release v.0.5.4 #734

Closed

1 task

smartie2076 reviewed Dec 18, 2020

View reviewed changes

mahendrark force-pushed the feature/stratified_therm_storage branch from 6998795 to 8a5e528 Compare December 22, 2020 18:37

Merge branch 'dev' into feature/stratified_therm_storage

043c61b

MaGering closed this Jan 5, 2021

MaGering deleted the feature/stratified_therm_storage branch January 5, 2021 18:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/stratified therm storage #711

Feature/stratified therm storage #711

MaGering commented Dec 11, 2020

smartie2076 left a comment •

edited

Loading

MaGering commented Dec 12, 2020 •

edited

Loading

MaGering commented Dec 12, 2020

smartie2076 commented Dec 12, 2020 •

edited

Loading

MaGering commented Dec 13, 2020

smartie2076 commented Dec 14, 2020

smartie2076 commented Dec 14, 2020 •

edited

Loading

MaGering commented Dec 15, 2020

smartie2076 left a comment

smartie2076 Dec 18, 2020

smartie2076 Dec 18, 2020

smartie2076 Dec 18, 2020

MaGering Dec 23, 2020

smartie2076 Dec 18, 2020

MaGering commented Dec 22, 2020 •

edited

Loading

smartie2076 commented Dec 22, 2020

mahendrark commented Dec 22, 2020 •

edited

Loading

MaGering commented Dec 22, 2020

smartie2076 commented Jan 5, 2021

MaGering commented Jan 5, 2021

	In order to model a stratified thermal energy storage the two parameters `fixed_losses_relative` and `fixed_losses_absolute` are passed through `storage_xx.csv`.
	In order to model a stratified thermal energy storage the two parameters `fixed_losses_relative` and `fixed_losses_absolute` are passed to `storage_xx.csv`.


		Please see the `oemof.thermal` `examples <https://github.com/oemof/oemof-thermal/tree/dev/examples/stratified_thermal_storage>`__ and the `documentation <https://oemof-thermal.readthedocs.io/en/latest/stratified_thermal_storage.html>`__ for further information.

		For an investment optimization the height of the storage should be left open in the precalculations and `installedCap` should be set to 0 or NaN.

		fixed_thermal_losses_relative,factor,0,,
		fixed_thermal_losses_absolute,kWh,0,,

Feature/stratified therm storage #711

Feature/stratified therm storage #711

Conversation

MaGering commented Dec 11, 2020

smartie2076 left a comment • edited Loading

Choose a reason for hiding this comment

MaGering commented Dec 12, 2020 • edited Loading

MaGering commented Dec 12, 2020

smartie2076 commented Dec 12, 2020 • edited Loading

MaGering commented Dec 13, 2020

smartie2076 commented Dec 14, 2020

smartie2076 commented Dec 14, 2020 • edited Loading

MaGering commented Dec 15, 2020

smartie2076 left a comment

Choose a reason for hiding this comment

smartie2076 Dec 18, 2020

Choose a reason for hiding this comment

smartie2076 Dec 18, 2020

Choose a reason for hiding this comment

smartie2076 Dec 18, 2020

Choose a reason for hiding this comment

MaGering Dec 23, 2020

Choose a reason for hiding this comment

smartie2076 Dec 18, 2020

Choose a reason for hiding this comment

MaGering commented Dec 22, 2020 • edited Loading

smartie2076 commented Dec 22, 2020

mahendrark commented Dec 22, 2020 • edited Loading

MaGering commented Dec 22, 2020

smartie2076 commented Jan 5, 2021

MaGering commented Jan 5, 2021

smartie2076 left a comment •

edited

Loading

MaGering commented Dec 12, 2020 •

edited

Loading

smartie2076 commented Dec 12, 2020 •

edited

Loading

smartie2076 commented Dec 14, 2020 •

edited

Loading

MaGering commented Dec 22, 2020 •

edited

Loading

mahendrark commented Dec 22, 2020 •

edited

Loading