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/feedin tariff alternative #685

Merged
merged 35 commits into from
Dec 8, 2020
Merged

Conversation

smartie2076
Copy link
Collaborator

@smartie2076 smartie2076 commented Dec 4, 2020

Fix #610

Changes proposed in this pull request:

  • set feed-in tariff to negative when executing C0.define_sink in C0.define_dso_sinks_and_sources
  • ❌ No adaptation of C1.check_feedin_tariff() necessary
  • added benchmark test for feed-in tariff
  • make sure that feedin revenue is indeed substracted from the operational costs
  • ❌ give a hint in readthedocs about feed-in tariff sign --> looks fine in rtd: "Price received for feeding electricity into the grid." implies that it's positive.

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 Author

@SabineHaas this PR can now be reviewed. I also added a lot of pytests, but if you review commit by commit everything should be clear.

@smartie2076
Copy link
Collaborator Author

I have not checked the investments and if the costs add up. Especially for the pie charts, the revenues from feedin might result in weird stuff. I would say though that we do that in a new PR, eg. when merging #613?

@SabineHaas
Copy link
Collaborator

Thank you for reworking @smartie2076. Unfortunately I still get the error mentioned in #670. Apart from that, two more benchmark tests fail now when I run EXECUTE_TESTS_ON=master pytest locally, see error log:
pytest_master_log.txt

Could you please run the test locally again and check?

@smartie2076
Copy link
Collaborator Author

Thank you for reworking @smartie2076. Unfortunately I still get the error mentioned in #670. Apart from that, two more benchmark tests fail now when I run EXECUTE_TESTS_ON=master pytest locally, see error log:
pytest_master_log.txt

Could you please run the test locally again and check?

So weird, when I ran them Friday they seemed to pass... will check what this is about.

@SabineHaas
Copy link
Collaborator

Thank you for reworking @smartie2076. Unfortunately I still get the error mentioned in #670. Apart from that, two more benchmark tests fail now when I run EXECUTE_TESTS_ON=master pytest locally, see error log:
pytest_master_log.txt
Could you please run the test locally again and check?

So weird, when I ran them Friday they seemed to pass... will check what this is about.

Thank you so much!

@smartie2076
Copy link
Collaborator Author

@SabineHaas fixed the pytests. Go commit by commit, because somehow the files that were changed in another PR actually appear in this one now...

@smartie2076
Copy link
Collaborator Author

smartie2076 commented Dec 8, 2020

@SabineHaas ready to be merged!

If there is still something wrong, I am really at a loss here, as my pytests run trough:=========================================================== 319 passed, 2 skipped, 14 warnings in 529.66s (0:08:49) ============================================================

@SabineHaas
Copy link
Collaborator

@SabineHaas ready to be merged!

If there is still something wrong, I am really at a loss here, as my pytests run trough:=========================================================== 319 passed, 2 skipped, 14 warnings in 529.66s (0:08:49) ============================================================

:D
I've checked it, it's fine 👍

@SabineHaas SabineHaas merged commit a7d7818 into dev Dec 8, 2020
@SabineHaas SabineHaas deleted the fix/feedin_tariff_alternative branch December 8, 2020 14:29
@SabineHaas SabineHaas mentioned this pull request Dec 8, 2020
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.

Feed-in tariff in 'energyProvider.csv' should be negative
2 participants