-
Notifications
You must be signed in to change notification settings - Fork 45
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
O&M Technologies Bug #333
O&M Technologies Bug #333
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this. One comment for consistency.
greenheart/tools/eco/finance.py
Outdated
battery_opex = hopp_results["hybrid_plant"].battery.om_fixed + np.sum( | ||
hopp_results["hybrid_plant"].battery.om_variable | ||
battery_opex = (np.sum(hopp_results["hybrid_plant"].battery.om_fixed) | ||
* hopp_config["technologies"]["battery"]["system_capacity_kw"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should still use the om_total_expense for the battery as you did in the PV for GreenHEART, but we should set the om_variable to 0 for the battery if we don't want any variable expense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good, thank! There are a few things I think we should take care of still. See my other comments.
"om_batt_fixed_cost" | ||
] = config.hopp_config["config"]["cost_info"]["battery_om_per_kw"] | ||
"om_capacity" | ||
][i] = config.hopp_config["config"]["cost_info"]["battery_om_per_kw"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should have the [i]
here unless this is supposed to be a loop like the pv
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works just like pv so it also needs to loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no loop. Can you add it?
@@ -337,24 +345,38 @@ def set_om_costs_per_kw(self, pv_om_per_kw=None, wind_om_per_kw=None, | |||
# raise ValueError(f"Length of yearly om cost per kw arrays must be equal. Some lengths of om_per_kw values are different from others: {om_lengths}") | |||
if pv_om_per_kw and self.pv: | |||
self.pv.om_capacity = pv_om_per_kw | |||
if pv_om_per_mwh: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these if statements are not quite right logically. I don't see any reason to make the per_hwh
calls dependent on the per_kw
calls. I think we should have something like the following (which also highlights it would be nice to only write this once and re-use the code rather than re-writing for every tech).
if self.tech:
if tech_om_per_kw:
self.tech.om_capacity = tech_om_per_kw
if tech_om_per_mwh:
self.tech.om_capacity = tech_om_per_kwh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agree that it would be great to do some tech abstraction so it doesn't have to get so long but there are several places in HybridSimulation
where I think that abstraction would be valuable. I don't think I have time right now to do that but I've made an issue about it #337
Fix O&M Technologies Bug
Solar and battery O&M was not being converted from the fixed O&M input in
cost_info
in$/kW-yr
to$/yr
required for ProFAST calculations.pv
change method to use first year ofom_total_expense
output.battery
change method to use multiplyom_fixed
andsystem_capacity_kw
. Did not include Variable O&M based on ATB.Related issue
Impacted areas of the software
Additional supporting information
Test results, if applicable