-
Notifications
You must be signed in to change notification settings - Fork 65
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
adding LCOW per unit and per flow #1398
adding LCOW per unit and per flow #1398
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.
Being able to grab the LCOW breakdown quickly without (or with minimal) manual intervention will be very useful. I have a couple of comments to consider.
if hasattr(u, "capital_cost"): | ||
capital_cost = pyo.units.convert(u.capital_cost, to_units=c_units) | ||
# total capital costs w/ recovery factor | ||
numerator += self.capital_recovery_factor * ( |
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.
Rather than add the capex and opex components to the numerator (which could be one level of aggregation), the more typical/informative convention for LCOW breakdown is to separately report the capex and opex components separately for each unit. I think summing the fixed and variable opex would be OK.
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.
What should the names be?
We can add unlimited expressions will little additional cost. I.e., we could do
LCOW_unit_total[ unit_name ]
, LCOW_unit_capex[ unit_name ]
, LCOW_unit_opex[ unit_name ]
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.
That's a good question. Maybe something like
LCOW_capex[unit_name_or_indirect_capex_component]
LCOW_opex[unitname_or_flow_name]
So LCOW_opex["electricity"] or LCOW_opex["reverse_osmosis"] would work.
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'm worried about name clashes if I put units and flows in the same object -- there are other checks to make sure unit names and flow names don't clash with each other. It would probably be best to keep them separate.
Note that flows, by definition, will only have an opex anyways.
pyo.Any, | ||
doc=f"Levelized Cost of Water per flow based on flow {flow_rate.name}", | ||
) | ||
self.add_component(name + "_per_flow", flow_lcows) |
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.
Personally, I think "per_flow" can be a little confusing to users with water treatment background, though I see how it ties to the jargon of the idaes costing framework. Ideally, we'd have something like "LCOW_comp_electricity" or "LCOW_comp["electricity"]", for example.
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 guess nothing else is indexed for the flow components, so LCOW_comp_electricity
seems okay to me.
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.
To my eyes/ears familiar with IDAES naming conventions, comp
in the name indicates a chemical (dissolved or otherwise) component... "per stream"?
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.
oops missed the outdated tag on this comment
for f in flows: | ||
# part of total_variable_operating_cost | ||
flow_cost = pyo.units.convert(f * cost_var, to_units=c_units / t_units) | ||
flow_lcows[str(f)] = (flow_cost * self.utilization_factor) / denominator |
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 suppose we should be aware of a potential edge case--where the primary treatment segment of a plant may be turned off, but there is still some minimum load for parts of the plant that stay on. Up until now, we've assumed that there is absolutely no electricity usage during shutdown when in fact, while we may not be producing treated water at a given time, we can still be, e.g., pretreating water with units that must stay on even when the primary treatment step shuts down.
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.
They'll get an error when evaluating this (and the other expressions) then. Not sure there's much to be done about it.
# part of total_variable_operating_cost | ||
flow_cost = pyo.units.convert(f * cost_var, to_units=c_units / t_units) | ||
flow_lcows[str(f)] = (flow_cost * self.utilization_factor) / denominator | ||
|
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.
Just want to add a note that we should eventually perform a similar unit breakdown for SEC.
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.
Yes -- once we settle on exactly what should be reported I will use the code here to do it for every "per flow" aggregate.
This reverts commit 7038557.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1398 +/- ##
=======================================
Coverage 93.92% 93.92%
=======================================
Files 335 335
Lines 35620 35655 +35
=======================================
+ Hits 33456 33489 +33
- Misses 2164 2166 +2 ☔ View full report in Codecov by Sentry. |
I'll just echo @adam-a-a's comments in general for this and am less concerned with the naming convention. I assume you could provide any flow expression you want (not clear if this was answered above)? e.g., |
@bknueven wants to test this again the latest idaes-pse |
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.
Math looks right. A few comments to consider.
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.
Thought of one more thing to consider before approving.
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.
Approved. Looking forward to using this feature.
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.
Tested this on complex flow sheet and looks good to me. I do wounder if this could benefit from a pretty print command that prints out all the breakdowns (instead of having to manually display each type) but not needed in this pr.
Fixes #1374
Summary/Motivation:
Add named
Expression
s to provide more breakdowns in aggregate costs.Changes proposed in this PR:
LCOW Aggregate Breakdown (LSRRO flowsheet):
LCOW Component Breakdown (LSRRO flowsheet):
SEC Breakdown (LSRRO Flowsheet):
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: