Skip to content

Write benchmark tests for npc and lcoe#613

Merged
smartie2076 merged 24 commits intodevfrom
feature/benchmark-npc-lcoe
Dec 11, 2020
Merged

Write benchmark tests for npc and lcoe#613
smartie2076 merged 24 commits intodevfrom
feature/benchmark-npc-lcoe

Conversation

@Bachibouzouk
Copy link
Copy Markdown
Collaborator

@Bachibouzouk Bachibouzouk commented Oct 21, 2020 •
edited by smartie2076
Loading

Fix #504
Fix #700
Fix #549

Changes proposed in this pull request:

  • Adapt the investment benchmark test so, that it checks LCOE as well as all other economic parameters are calculated correctly (for a single-vector system)
  • Change pre-processing for investment benchmark tests so, that it can be executed as a seperate function
  • Change parameters that are included in output cost matrix: Add COST_REPLACEMENT
  • Fix: Add assertion sum(attributed_costs)==cost_total

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)

Edited by @smartie2076 on 2020-12-10

@Bachibouzouk Bachibouzouk marked this pull request as draft October 21, 2020 09:11
@Bachibouzouk
Copy link
Copy Markdown
Collaborator Author

Bachibouzouk commented Oct 21, 2020

@smartie2076 - can you tell me if this is what you had in mind?

smartie2076
smartie2076 reviewed Oct 21, 2020
View reviewed changes
Copy link
Copy Markdown
Collaborator

@smartie2076 smartie2076 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, that is the right direction. For now, you tidied up the data and avoided the python processing by writing the expected values for the individual assets to file. Basically, my script then is used as an external scipt, and now can be deleted.

Missing is now the calculation of the global economic parameters (annuities, costs, lcoe).

@Bachibouzouk Bachibouzouk force-pushed the feature/benchmark-npc-lcoe branch 2 times, most recently from 65000ce to db5524a Compare October 21, 2020 17:50
smartie2076
smartie2076 reviewed Oct 23, 2020
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.

Looking good! I think you could also check for some more parameters - and I hope that with this you got to know the economic KPI a bit better :)

@smartie2076 smartie2076 mentioned this pull request Nov 3, 2020
Closed
30 tasks
@smartie2076
Copy link
Copy Markdown
Collaborator

smartie2076 commented Nov 4, 2020

Hi @Bachibouzouk! What should I review here? My last review is still up-to-date, as you did not push anything since.

@smartie2076 smartie2076 added the enhancement New feature or request label Nov 4, 2020
@Bachibouzouk
Copy link
Copy Markdown
Collaborator Author

Bachibouzouk commented Nov 4, 2020 •
edited
Loading

Missing is now the calculation of the global economic parameters (annuities, costs, lcoe).

@smartie2076 I thought the PR was only for NPC and LCOE ?

@smartie2076
Copy link
Copy Markdown
Collaborator

smartie2076 commented Nov 4, 2020

Missing is now the calculation of the global economic parameters (annuities, costs, lcoe).

@smartie2076 I thought the PR was only for NPC and LCOE ?

Verbally, we identified a list of cost parameters that could easily be added to the check. I think the PR is called that way because Benchmark test for npc, lcoe, cost total, cost om, annuity total, annuity om would be a bit long ;)

@smartie2076
Copy link
Copy Markdown
Collaborator

smartie2076 commented Nov 4, 2020

We can do it in a seperate PR, if you prefer. Can you add the boxes then in the original issue?

@smartie2076 smartie2076 mentioned this pull request Nov 7, 2020
Closed
36 tasks
@smartie2076 smartie2076 mentioned this pull request Nov 19, 2020
Closed
13 tasks
@smartie2076 smartie2076 mentioned this pull request Dec 4, 2020
Merged
12 tasks
@smartie2076
Copy link
Copy Markdown
Collaborator

smartie2076 commented Dec 9, 2020

I will take over this PR now, also directly pushing into it.

@smartie2076 smartie2076 force-pushed the feature/benchmark-npc-lcoe branch from 6b096db to 78425f8 Compare December 9, 2020 15:32
@smartie2076 smartie2076 mentioned this pull request Dec 9, 2020
Closed
9 tasks
@smartie2076 smartie2076 marked this pull request as ready for review December 10, 2020 12:15
@smartie2076
Copy link
Copy Markdown
Collaborator

smartie2076 commented Dec 10, 2020

@SabineHaas this PR is ready for review.

SabineHaas
SabineHaas approved these changes Dec 11, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@SabineHaas SabineHaas left a comment

Choose a reason for hiding this comment

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

@smartie2076 I only have minor comments, so I already approve.

However, important!, for your information:
The second test in test_benchmark_KPI.py is not tested, as the prefix test_ is missing, see here

@smartie2076
Copy link
Copy Markdown
Collaborator

smartie2076 commented Dec 11, 2020

However, important!, for your information:
The second test in test_benchmark_KPI.py is not tested, as the prefix test_ is missing, see here

Oh no! I hope it works out of the box...

@smartie2076 smartie2076 merged commit fcd5f1b into dev Dec 11, 2020
@smartie2076 smartie2076 deleted the feature/benchmark-npc-lcoe branch December 11, 2020 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@smartie2076 smartie2076 smartie2076 left review comments

+1 more reviewer

@SabineHaas SabineHaas SabineHaas approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Improve investment benchmark tests [Bug] Need for an assertion of sum(attributed costs)==total_costs Write benchmark tests for investment model

4 participants

@Bachibouzouk @smartie2076 @SabineHaas @mahendrark