Migrate costing metrics from valorization_costing_block - #1764
Migrate costing metrics from valorization_costing_block#1764luohezhiming wants to merge 26 commits into
Conversation
|
I suggest consolidating mass-based and volumetric methods into shared basis-aware implementation for each metric, so the same methods can handle either flow basis through Possible culprits:
Follow-up actions:
|
Co-authored-by: Carolina Tristan <ctristan@purdue.edu>
Co-authored-by: Carolina Tristan <ctristan@purdue.edu>
Co-authored-by: Carolina Tristan <ctristan@purdue.edu>
Co-authored-by: Carolina Tristan <ctristan@purdue.edu>
I think keep add_LCOP and SEC with respect to product will be more reasonable, it's not only we want to account for volumetric and mass flow, the major difference is to base on product or water, and usually the product is mass based, the product can be any byproducts like if we can recovering nutrients. Then we devide by the mass flow of nutrients, the performance index like levelized cost of product should not be called LCOW anymore, it is supposed to be called LCOP |
@tristantc for LCOP and annual_product_generation, please double check their performance indices name, I don't think user should expect to use add_LCOW function to add LCOP and add_annual_water_production function for adding product generation. The major difference is not only the volumetric-base and mass-base, the product does not need to be water, user will not use add_annual_water_production to add generation for product. maybe the specific_energy_consumption we should change the name, but I think we still need to keep different functions |
|
@luohezhiming, movnig this to Sept, speak up if you disagree. |
…or optional parameters
… `add_annual_total` method
…is handling and update documentation
…on for flow_basis
| "'volumetric', 'mass', and 'energy'." | ||
| ) | ||
|
|
||
| flow_units = { |
There was a problem hiding this comment.
why not add these as a property of the class like we have base_currency and base_period? A user might want to use a different basis for flow.
| """ | ||
| return self.add_annual_total(flow_rate, flow_basis="volumetric", name=name) | ||
|
|
||
| def add_annual_total(self, flow_rate, flow_basis="volumetric", name="annual_total"): |
There was a problem hiding this comment.
add_annual_total_product is a better name imo; water is still a product.
There was a problem hiding this comment.
Not restricted to product; could be any annual total
kurbansitterley
left a comment
There was a problem hiding this comment.
- Is the purpose of these changes only to add levelized metrics based on mass?
- I would prefer if any changes in this PR goes along with proper documentation changes (in this PR).
| m.fs.product.properties[0].flow_vol, | ||
| flow_basis="volumetric", |
There was a problem hiding this comment.
add test to check what happens when flow basis is different from actual variable supplied (e.g., flow_vol, flow_basis="mass")
…om/luohezhiming/watertap into migrate_valorization_costing_block
… call sites The PR #1764 (migrate_valorization_costing_block) changed the signature of add_flow_component_breakdown() to require 'name' as 2nd positional arg and 'flow_rate' as 3rd, but forgot to update docs/how_to_guides/how_to_use_watertap_costing.rst. This commit applies all PR changes including: - New _FLOW_BASIS_KEYWORDS and _check_flow_basis_consistency for flow basis validation - New add_levelized_cost method supporting volumetric/mass/energy flow bases - New add_annual_total method - Updated add_specific_energy_consumption with flow_basis parameter - Updated add_flow_component_breakdown with new required 'name' arg as 2nd positional - Updated add_specific_electrical_carbon_intensity internal call - Updated ion_exchange_demo.py call - Updated test_costing_base.py (new test_flow_basis_mismatch replaces test_breakdowns_with_no_unit) - Fixed all 4 calls in how_to_use_watertap_costing.rst (the missing fix causing CI failure) - Updated code-block example in costing_base.rst
tristantc
left a comment
There was a problem hiding this comment.
LGTM, but we should make sure to include documentation for the updated cost metrics in a later PR
| name, | ||
| pyo.Expression( | ||
| expr=( | ||
| self.total_capital_cost * self.capital_recovery_factor |
There was a problem hiding this comment.
So, we actually have total_annualized_cost Expression that we can simply use here in stread-> will make it alot more clear.
There was a problem hiding this comment.
So then here we would simply
self.add_component(
name,
pyo.Expression(
expr=pyunits.convert((self.total_annualzied_cost/(flow_rate*utiliation_factor), to_unit=normalized_units[flow_type]))
Where normalzied units is either pre-registerd dict unit or user supplied unit in cost/amount format
so we would need
normalized_units={'mass':self.base_currency/pyunits.kg... etc}
Then if user provides a desired unit, we use it instead of the one registed.
| ) | ||
| self._check_flow_basis_consistency(flow_rate, flow_basis) | ||
|
|
||
| flow_units = { |
There was a problem hiding this comment.
We should really let user define final desired units USD/kg as well as an option. I also think, it makes more sense to instead of covnerting the flow to instead enforce final conversion (e.g we should define things on final cost basis (USD/kg ,USD/m3 etc. Ideally we should simply pull out actual mass/flow basis unit from the Var being passed but could not finda simple way to do it.
| ) | ||
| self._check_flow_basis_consistency(flow_rate, flow_basis) | ||
|
|
||
| flow_units = { |
There was a problem hiding this comment.
Same as above, user should supply desired final unit (kW/kg etc. we should also do conversion and defin self.base_energy_units -> actually, it will be way better if we create bese units for costing and use them through out instead of manually defining them - we then should create a function that will return the right base untis for conversion. e.g. def get_normalzied_cost(self, type='volume'): -> returns self.base_currancy/self.base_volume
etc.
avdudchenko
left a comment
There was a problem hiding this comment.
This is overall misses a number of improvements and simplifications in implementation.
(1) We should commit to defining base units in costing for flow, mass, power etc. same as we do for cost and base_period, we should be consistent.
(2) We should have a standard function to get normalized units that is used by levelized/total methods. This way we do not constantly defining unit dicts everywhere.
This also means a user can change base untis as they see fit - .e.g we should have a way for user to specify base units for costing after creation (m.fs.costing() - or via config - pick one...) m.fs.costing.set_base_cost_untis('USD_2024), set_base_power_units('kW') etc.
This levelized functions should be applying a conversion to levelized value in final expression, rather just converting the flow rate. - the user should also be able to specify the units (USD/kg as they fit. We should also run check_units_consistent on expression build, to ensrue its all good.
Fixes/Resolves:
(replace this with the issue # fixed or resolved, if no issue exists then a brief statement of what this PR does)
Summary/Motivation:
Migrate costing metrics from valorization_costing_block from brackish_valorization_reaktoro
Changes proposed in this PR:
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: