Skip to content

Correct weighting factors#894

Merged
smartie2076 merged 5 commits intodevfrom
rtd/weighting_factors
May 31, 2021
Merged

Correct weighting factors#894
smartie2076 merged 5 commits intodevfrom
rtd/weighting_factors

Conversation

@TheOneAndra
Copy link
Copy Markdown
Collaborator

@TheOneAndra TheOneAndra commented May 25, 2021 •
edited by smartie2076
Loading

Fix #822

Changes proposed in this pull request:

  • Correct the weighting factors

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.

@TheOneAndra
Copy link
Copy Markdown
Collaborator Author

TheOneAndra commented May 25, 2021 •
edited
Loading

I am not sure how to tackle issue #822 with the natural gas energy carrier.
Should we change the table so that the user uses gas as energy vector for natural gas or should we change the energy carrier name in the MVS so that natural_gas is accepted ?

any suggestion @smartie2076 ?

@smartie2076
Copy link
Copy Markdown
Collaborator

smartie2076 commented May 27, 2021

@TheOneAndra I had to update as there was a dev fix

@smartie2076 smartie2076 self-requested a review May 27, 2021 12:47
@smartie2076
Copy link
Copy Markdown
Collaborator

smartie2076 commented May 27, 2021 •
edited
Loading

Hi @TheOneAndra! Hm, this is also a question of an API change - when we change from Gas to Natural_gas some previously created simulations may not work anymore (also, the EPA uses Gas now). So eventhough Natural_gas is more explicit, I would not change the name in constants.py. How about you change the tables in https://multi-vector-simulator.readthedocs.io/en/latest/model/assumptions.html#weighting-of-energy-carriers and make Gas oil/diesel --> Diesel and Natural gas --> Gas?
Maybe you can also add a warning like this at the end of the section?

.. note::
   The :code:`energy_vector` of each of the assets and busses must be identical in spelling to one of the energy carriers defined in the above table. Other energy carriers can not be parsed and will raise a warning. Please note that `Heat` currently has to be measured in kWh(thermal).

smartie2076
smartie2076 reviewed May 27, 2021
View reviewed changes
Copy link
Copy Markdown
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.

Update changelog (Changed and fixed section), write Fix #822 in first post to close issue automatically.

Check src/utils/constants.py and DEFAULT_WEIGHTS_ENERGY_CARRIERS whether the names of the energy carriers are identical, and if any are missing. I think Crude_Oil for example is different, maybe we can rephrase to Oil only?

Can you send me the link to the excel file again via rocketchat, so that I can cross-check properly this time around?

@smartie2076
Copy link
Copy Markdown
Collaborator

smartie2076 commented May 31, 2021

Hi @TheOneAndra! Hm, this is also a question of an API change - when we change from Gas to Natural_gas some previously created simulations may not work anymore (also, the EPA uses Gas now). So eventhough Natural_gas is more explicit, I would not change the name in constants.py. How about you change the tables in https://multi-vector-simulator.readthedocs.io/en/latest/model/assumptions.html#weighting-of-energy-carriers and make Gas oil/diesel --> Diesel and Natural gas --> Gas?

Added Natural_gas, identical to Gas, as EPA needs to use Gas. It does not matter which one is used.

Maybe you can also add a warning like this at the end of the section?

Added comment

TheOneAndra and others added 4 commits May 31, 2021 13:31
@TheOneAndra @smartie2076
Fix and round weighting factors
425695d
@smartie2076
Update notes in RTD regarding energy carrier weighting
6867652 
- weighting factor approx. LHV, consideration to change it to LHV in future
- Note on energy carrier names in RTD
@smartie2076
Fix energy carrier weighting factors in utils/constants.py
0d7186d 
- Rename `Crude_Oil` to `Crude_oil`
- Add `Natural_gas` next to `Gas` (`Gas` is used in EPA)
- Change values (higher decimal numbers then in `assumptions.rst`)
@smartie2076
Update changelog
28df015
@smartie2076 smartie2076 force-pushed the rtd/weighting_factors branch from 042848c to 28df015 Compare May 31, 2021 11:33
@smartie2076 smartie2076 mentioned this pull request May 31, 2021
Closed
smartie2076
smartie2076 approved these changes May 31, 2021
View reviewed changes
Copy link
Copy Markdown
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.

I approve ;)

@smartie2076 smartie2076 merged commit d8eedf2 into dev May 31, 2021
@smartie2076 smartie2076 deleted the rtd/weighting_factors branch May 31, 2021 11:44
@smartie2076 smartie2076 mentioned this pull request May 31, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@smartie2076 smartie2076 smartie2076 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Natural gas energy carrier

2 participants

@TheOneAndra @smartie2076