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

Lcoe branch #1687

Open
wants to merge 50 commits into
base: main
Choose a base branch
from
Open

Lcoe branch #1687

wants to merge 50 commits into from

Conversation

eccoope
Copy link

@eccoope eccoope commented Mar 6, 2023

  • Closes LCOE function #1667
  • I am familiar with the contributing guidelines
  • Tests added
  • Updates entries in docs/sphinx/source/reference for API changes.
  • Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for all changes. Includes link to the GitHub Issue with :issue:`num` or this Pull Request with :pull:`num`. Includes contributor name and/or GitHub username (link with :ghuser:`user`).
  • New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

@eccoope
Copy link
Author

eccoope commented Mar 6, 2023

Financial module - includes LCOE function and supporting financial functions - with unit tests and two examples.

@mikofski
Copy link
Member

mikofski commented Mar 7, 2023

Great start @eccoope. Very happy to see your contribution. There are a number of stickler issues. Can you please resolve them so it will be easier to review?

Copy link
Member

@mikofski mikofski left a comment

Choose a reason for hiding this comment

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

You might consider converting to draft?

pvlib/financial.py Outdated Show resolved Hide resolved
pvlib/financial.py Outdated Show resolved Hide resolved
pvlib/financial.py Outdated Show resolved Hide resolved
pvlib/financial.py Outdated Show resolved Hide resolved
@eccoope eccoope marked this pull request as draft March 7, 2023 14:37
@kandersolar kandersolar mentioned this pull request Mar 7, 2023
eccoope and others added 7 commits March 7, 2023 11:39
mikeofski recommended not rounding returns in the financial.py module, tests needed to be updated for expected to match out
@mikofski
Copy link
Member

mikofski commented Mar 8, 2023

A few more stickler errors. I know they're a drag, that's why it's called the "stickler" 😆 Also, tiny nit, not a huge blocker, but please try to use a message for every commit. Even better if it's unique and meaningful. Thanks! These messages can help in reviews. 😄

pvlib/financial.py Outdated Show resolved Hide resolved
@cwhanse
Copy link
Member

cwhanse commented Mar 14, 2023

@eccoope please add a line to https://github.com/pvlib/pvlib-python/blob/main/docs/sphinx/source/reference/index.rst so that the new page builds in the API reference.

@mikofski
Copy link
Member

mikofski commented Mar 16, 2023

@eccoope you'll need to resolve the conflict in the "What's new" for v0.9.5. This happens often unfortunately because it's one of the few files that gets edited simultaneously by more than one PR. In this case I think it's easy to fix -- you just want your name to appear AND the other two contributors as well who are already in the main branch's version of v0.9.5.rst
image

It's entirely possible that just sync'ing your main branch, merging it into this lcoe_branch locally, and then pushing it up to GH will resolve this automatically. If you already have experience with this I apologize for being redundant. I've found that GH merge strategy on PR's is a bit more conservative than running Git locally on my machine.

@mikofski
Copy link
Member

ack please merge main again, as more PR's are merged getting close to release you'll have to keep sync'ing more often, sorry:
image

Again so super sorry if I'm being redundant, but these commands locally should work:

# from a clean repo
(lcoe_branch) pvlib\$ git checkout main
# assuming upstream remote = https://github.com/pvlib/pvlib-python
(main) pvlib\$ git pull upstream main 
(main) pvlib\$ git checkout lcoe_branch
(lcoe_branch) pvlib\$ git merge main
# assuming your local branch is tracking your remote fork
(lcoe_branch) pvlib\$ git push

@wholmgren wholmgren mentioned this pull request Mar 17, 2023
12 tasks
@mikofski
Copy link
Member

Hi @eccoope, to have more time to build consensus on this PR, we're pushing it to the next version so that v0.9.5 can be released now. This will mean some editing your part to update the What's new entry for v0.10.0. Thanks!

@mikofski
Copy link
Member

@eccoope when you get a chance, can you merge master into your branch, resolve the what's new conflicts (sorry this always happens) maybe move them to v0.9.6 or v0.10.0, and resync? thanks! I am hoping we are all on schedule to push this in the next major release? I could use it even sooner.

Real levelized cost of electricity (LCOE).

Includes cost of capital and fixed operations and maintenance (O&M), but
not variable O&M. Described in [1]_, pp. 43 and 47-48, and defined here as
Copy link
Member

Choose a reason for hiding this comment

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

something I learned recently is that the variable o&m cost is zero for solar and typ. refers to the cost of fuel for fossil fuel power

Copy link
Member

Choose a reason for hiding this comment

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

also, if I understand correctly both the simple and fixed charge rate LCOE models assume that the degradation rate is the same as the discount rate or the cost of capital, which is probably conservative. Deg rates are typically less than 1% while the suggested discount rate is 3%. The FCR simplification is:

LCOE = (FCR*CAPEX + OPEX_FIX) / YIELD + OPEX_VAR  # OPEX_VAR = zero for solar!

which is equivalent to:

LCOE = (CAPEX + OPEX/FCR) / (YIELD/FCR)  # just use OPEX for fixed O&M

I believe we can do better than this by just using either the lifetime production in the denominator or using a different FCR for production:

LCOE = (CAPEX + OPEX/FCR) / (YIELD/FIXDEGRATE)
# where FIXDEGRATE = [1 / SUM(1 / (1+RD)**n) for n in range(N)]
# N is lifetime in years, same as used for fixed charge rate
# and RD is the degradation rate, usually 0.75%/year

I think this might return an LCOE with more realistic accounting for degradation?

Copy link

@brtietz brtietz Jun 28, 2023

Choose a reason for hiding this comment

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

The LCOE models apply the financial discount rate to the future energy yield, see equation 4-6 of https://www.nrel.gov/docs/legosti/old/5173.pdf This allows future costs and future energy to be on the same basis.

PV degradation needs to be included in the yield number. For a discounted cash flow we use equation 4-6 (the sum with specific values for each year). For the simple LCOE formula the ATB uses an average capacity factor over the life of the system.

Copy link
Member

Choose a reason for hiding this comment

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

I didn’t realize the FCR applies to the energy as well, I guess it makes sense that it’s value changes over time because of inflation.

okay so then the average yield should be the total production including degradation divided by the lifetime of the system? Thanks!

Copy link

Choose a reason for hiding this comment

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

okay so then the average yield should be the total production including degradation divided by the lifetime of the system? Thanks!

That's correct!

Copy link
Member

@cwhanse cwhanse Jun 28, 2023

Choose a reason for hiding this comment

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

Not to dampen enthusiasm, but does the Short et al. reference include degradation? I know that including degradation may give a more meaningful LCOE, but here, we're implementing what's been published. The function can be extended or modified to include degradation but I'd like to see a second reference describing how that's done.

Copy link
Member

Choose a reason for hiding this comment

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

@eccoope I think there should be something in the docs or example that shows that the fuel cost is zero for solar. Perhaps other newbies to financing like me will also be confused

Copy link

Choose a reason for hiding this comment

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

Not to dampen enthusiasm, but does the Short et al. reference include degradation? I know that including degradation may give a more meaningful LCOE, but here, we're implementing what's been published. The function can be extended or modified to include degradation but I'd like to see a second reference describing how that's done.

Looking at the code again, given that production is an array, it might be better to have users include degradation in that array. In that case, equation 4-6 for Short et al. still works. If you prefer a year 1 energy yield and a degradation factor, equation 2 of https://pubs.rsc.org/en/content/articlehtml/2011/ee/c0ee00698j provides a really good example.

If there is demand for the average yield approach I can ping the PV team for the ATB.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @mikofski ! Degradation rates are implemented as an average capacity factor from the ATB as mentioned by @brtietz in simple_lcoe_calculator.py (but I could probably be more explicit about this in a comment), but there's also a discount rate of 3% in this example which is separate from degradation.

Degradation is not accounted for in docs/examples/financial/lcoe_sam_validation.py - constant AC power is explicitly assumed (see line 126) - but we could easily incorporate by multiplying the production array element-wise with a degradation array like the one you defined as FIXEDEGRATE.

@mikofski and @brtietz - if either of you would be interested in meeting with me and possibly @cwhanse for 30 minutes to clarify any of these details, let me know

Copy link

Choose a reason for hiding this comment

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

@eccoope Sure - please email me at brian.mirletz (at) nrel (dot) gov to set up a time.

@kandersolar kandersolar modified the milestones: 0.10.0, v0.10.2 Jul 6, 2023
@kandersolar kandersolar modified the milestones: v0.10.2, Someday Sep 21, 2023
@jules-ch
Copy link
Contributor

is there any news on merging this, any required change or should it be impelmented on another lib to ease maintainability ?

@cwhanse
Copy link
Member

cwhanse commented Dec 12, 2023

is there any news on merging this, any required change or should it be impelmented on another lib to ease maintainability ?

The submitter appears to have moved on to other work. My read is that there is still work to be done here before we get agreement to merge, so looking for a volunteer to adopt this PR.

@eccoope
Copy link
Author

eccoope commented Dec 13, 2023

is there any news on merging this, any required change or should it be impelmented on another lib to ease maintainability ?

The submitter appears to have moved on to other work. My read is that there is still work to be done here before we get agreement to merge, so looking for a volunteer to adopt this PR.

@cwhanse what are you envisioning for additional work, besides resolving merge conflicts as they arise?

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

Successfully merging this pull request may close these issues.

LCOE function
6 participants