Skip to content

Comments

Fix incorrect use of LCOE and ill-defined example#304

Merged
dalonsoa merged 10 commits intodevelopfrom
lcoe
May 16, 2024
Merged

Fix incorrect use of LCOE and ill-defined example#304
dalonsoa merged 10 commits intodevelopfrom
lcoe

Conversation

@dalonsoa
Copy link
Collaborator

Description

This PR fixes a couple of problems:

  1. The LCOE now returns a 0 for those timeslices where the utilization factor is zero - which would result in an infinity LCOE as a result. Fixes [BUG] LOCE calculation is incorrect when more than one time slice is present #246 .
  2. The default_timeslice model was ill-defined, with wrong values for the MinimumServiceFactor, which must be between 0 and 1, inclusive.
  3. Fixes a weird bug that was reveled when changing the above two. It's unclear why things were not failing before, but I suspect that another, unrelated error was causing the associated tests to pass. The real problem is that the tests that were supposed to check this are really more integration tests that actual unit tests, so a lot of things were going on.

Type of change

Please add a line in the relevant section of
CHANGELOG.md to
document the change (include PR #) - note reverse order of PR #s.

  • New feature (non-breaking change which adds functionality)
  • Optimization (non-breaking, back-end change that speeds up the code)
  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (whatever its nature)

Key checklist

  • All tests pass: $ python -m pytest
  • The documentation builds and looks OK: $ python -m sphinx -b html docs docs/build

Further checks

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

@ahawkes
Copy link
Collaborator

ahawkes commented May 14, 2024 via email

@alexdewar
Copy link
Collaborator

One regression test is failing for all the runners... I'm guessing we're getting a different result now because the calculation has changed @dalonsoa?

@alexdewar alexdewar changed the title Fix iincorrect use of LCOE and ill-defined example Fix incorrect use of LCOE and ill-defined example May 14, 2024
Copy link
Collaborator

@alexdewar alexdewar left a comment

Choose a reason for hiding this comment

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

A few minor suggestions, but otherwise all good!

@dalonsoa
Copy link
Collaborator Author

Thanks this looks good to me. Is there another issue required to fix the input data on the time slice problem?

As far as I can tell, that's all. The results seems to make sense and once the minimum service issue is solved and meaningful values are provided for it, the calculation completes even with some utilization factors equal to zero, which I think was the next main barrier.

I would suggest we merge this, create a release and you try to use it to check that it produces sound results.

@ahawkes
Copy link
Collaborator

ahawkes commented May 15, 2024 via email

@dalonsoa
Copy link
Collaborator Author

Note, however, that the overall approach is still lacking despite this fix. This is because LCOE (and presumably other objectives) is calculated based on maximum utilisation. But when solved, actual utilisation could be a lot lower (e.g. the technology might be used only in 1 time slice), thus increasing LCOE substantially, potentially (likely!) making the technology choice wrong ex-post of the dispatch.

I'm quite unsure on how to implement this since actual utilisation won't be known until dispatch, which happens after the investment. Do you have any suggestion on how this could be done?

@ahawkes
Copy link
Collaborator

ahawkes commented May 15, 2024 via email

@dalonsoa
Copy link
Collaborator Author

@tsmbland it might be simpler if you review this and we merge it, and then you update your branch with develop. Splitting it into smaller bits is not going to help was you still need to bring all changes to your branch, anyway.

Copy link
Collaborator

@tsmbland tsmbland left a comment

Choose a reason for hiding this comment

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

Looks good to me. I think there's only one small change I'll have to make to one of the tutorials

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[BUG] LOCE calculation is incorrect when more than one time slice is present

4 participants