Skip to content

Conversation

lixiangk1
Copy link
Collaborator

@lixiangk1 lixiangk1 commented Feb 29, 2024

  • Add battery O&M in $ / kW-installed and $ / kWh-installed, defaulted to $0
  • Add battery self-discharge as a fraction per day loss based on stored kWh, defaulted to 0.0

model_degradation::Bool = false
degradation::Dict = Dict()
minimum_avg_soc_fraction::Float64 = 0.0
daily_self_discharge_fraction::Float64 = 0.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

@lixiangk1 could you add a little explanatory text here for this input?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hey! Did you mean add notes in the code to better define this term or just explain it in the pull request?

The input is a loss term for battery self-discharge, entered as a daily fraction for how much of the stored kWh is leaked from the battery (e.g., 3% of the the kWh stored in the battery is self-discharged per day). This is then modeled as a per timestep loss term where eg. 0.03/24/timesteps_per_hour * dvStoredEnergy is subtracted from dvStoredEnergy at every timestep.

Copy link
Collaborator

@adfarth adfarth Mar 19, 2024

Choose a reason for hiding this comment

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

I mean adding a comment "# explanation" in the code here so that it appears in the REopt.jl documentation (https://nrel.github.io/REopt.jl/dev/reopt/inputs/#ElectricStorage). We haven't really done this for ElectricStorage inputs but for all of the other techs we've been including some help text around the inputs. If you could do the same for the O&M costs that would be great! Maybe with a note about not double-counting replacement and O&M costs.

@adfarth
Copy link
Collaborator

adfarth commented Mar 19, 2024

@lixiangk1 Three quick comments from an initial review!

  • Could you please update the CHANGELOG?
  • Could you investigate why the tests are failing and try to get all tests to pass?
  • Could you add a test to runtests.jl for both the O&M costs and the self-discharge?

@lixiangk1
Copy link
Collaborator Author

@adfarth Thanks for your review! Do the latest changes address your comments? As for the tests, it seems that the Windows/MacOS ones are passing but the Ubuntu ones timeout - do you know why that might be happening? Is it something specific to this branch?

@adfarth
Copy link
Collaborator

adfarth commented Apr 5, 2024

@adfarth Thanks for your review! Do the latest changes address your comments? As for the tests, it seems that the Windows/MacOS ones are passing but the Ubuntu ones timeout - do you know why that might be happening? Is it something specific to this branch?

Recapping our convo here!

  • Expected that the ubuntu tests will fail so if the others are passing it's all good (but my latest merge of develop into this branch did seem to cause some to break :( )
  • Going to double check that a "self discharge fraction based on stored energy per time step" makes practical sense
  • Then, rename variable and help text, adjust calculation in the constraints
  • Test/document difference in results if discharge is applied to previous timestep's stored energy vs current one

@Bill-Becker
Copy link
Collaborator

@lixiangk1 any plans to finish this? I could probably support with a charge code in Q1.

@Bill-Becker
Copy link
Collaborator

@lixiangk1 just a note that we'll have to add the battery O&M to the proforma.jl functions to calculate e.g. simply payback and IRR.

@Bill-Becker
Copy link
Collaborator

@adfarth did you re-review this after @lixiangk1 's responses/updates after your initial review?

@adfarth
Copy link
Collaborator

adfarth commented May 30, 2025

@adfarth did you re-review this after @lixiangk1 's responses/updates after your initial review?

I haven't re-reviewed since the April 5, 2024 comment

- m[Symbol("dvDischargeFromStorage"*_n)][b,ts] / p.s.storage.attr[b].discharge_efficiency
)
- (p.s.storage.attr[b].soc_based_per_ts_self_discharge_fraction * m[Symbol("dvStoredEnergy"*_n)][b, ts])
- (p.s.storage.attr[b].capacity_based_per_ts_self_discharge_fraction * m[Symbol("dvStorageEnergy"*_n)][b])
Copy link
Collaborator

Choose a reason for hiding this comment

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

@lixiangk1 based on our convo last week - this "per_ts" method was also going to be adopted for H2, right? (instead of "daily leakage"). I agree with that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup, will be updating all the daily values to per ts. What do you think about having both an soc and capacity based leakage term for electric storage, thermal storage and h2?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems good to have both options, if it's not that much more development. I would imagine SOC-based could slow things down more.

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.

4 participants