Costing Unification: move from `factor_total_investment` to `TIC` #1175

bknueven · 2023-10-19T02:20:58Z

Fixes/Resolves: Part 1 of 3 for #1134

Summary/Motivation:

The two costing packages have different ways of accounting for indirect capital costs. The ZO Costing Package allows the indirect capital cost to be specified per unit model. The WT Costing Package has a global-level indirect capital cost.

This PR would resolve the conflict in favor of the ZO costing package. This requires modifying WaterTAP's existing detailed model's costing methods, and a few related changes to our detailed flowsheets.

Changes proposed in this PR:

Move TIC / TPEC factor calculation to the costing base class
Add TIC factor to all unit models which do not already have a factor
Specify a default o 2 for TIC when used in the WT Costing Package
Make factor_total_investment have a default value of 1 in the WT Costing Package
Various changes to detailed flowsheets to reproduce existing results.
Set automatic scaling factors on unit model costing aggregates to resolve some resulting detailed flowsheet instability (in particular, NF and LSRRO).

Legal Acknowledgement

By contributing to this software project, I agree to the following terms and conditions for my contribution:

I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

codecov · 2023-10-19T03:21:26Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (a88cf2f) 94.78% compared to head (8b34ef1) 94.77%.

Files	Patch %	Lines
watertap/costing/costing_base.py	90.47%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1175      +/-   ##
==========================================
- Coverage   94.78%   94.77%   -0.02%     
==========================================
  Files         356      356              
  Lines       35526    35553      +27     
==========================================
+ Hits        33675    33696      +21     
- Misses       1851     1857       +6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

avdudchenko

This looks good to me.

kurbansitterley · 2023-11-09T21:32:28Z

watertap/costing/unit_models/ion_exchange.py

    blk.capital_cost_constraint = pyo.Constraint(
        expr=blk.capital_cost
-        == pyo.units.convert(
+        == blk.cost_factor


There is currently a total_installed_cost_factor with value of 1.65 used in capital costs for IX that I had included before there was a global TIC. That should be removed entirely and use the global variable to not be double counting.

Of course whatever you decide to do it will cause the IX test to fail.

kurbansitterley · 2023-11-09T21:37:16Z

watertap/costing/unit_models/ion_exchange.py

@@ -336,9 +336,11 @@ def cost_ion_exchange(blk):
            to_units=blk.costing_package.base_currency,
        )
    )
+    blk.costing_package.add_cost_factor(blk, "TIC")


Suggested change

blk.costing_package.add_cost_factor(blk, "TIC")

blk.costing_package.add_cost_factor(blk, None)

I suppose we could also just use the local IX total_installed_cost_factor doing something like this? I am fine with the system TIC also and would let you make the decision.

Okay, thanks for catching this. I will use the system TIC and modify IX costing model appropriately.

kurbansitterley

There is a change needed for the ion_exchange costing model to not double count indirect costs with the use of TIC. As far as I know, that is the only unit model that had a local factor to account or indirect costs.

kurbansitterley · 2023-11-09T21:39:03Z

watertap/costing/unit_models/ion_exchange.py

    blk.capital_cost_constraint = pyo.Constraint(
        expr=blk.capital_cost
-        == pyo.units.convert(
+        == blk.cost_factor


Of course whatever you decide to do it will cause the IX test to fail.

kurbansitterley · 2023-11-09T21:43:09Z

watertap/unit_models/tests/test_anaerobic_digestor.py

@@ -407,7 +407,7 @@ def test_costing(self, adm):
        assert_optimal_termination(results)

        # Check solutions
-        assert pytest.approx(1083290.8, rel=1e-5) == value(
+        assert pytest.approx(2.0 * 1083290.8, rel=1e-5) == value(


How does doubling the capital costs via this factor not affect LCOW? I don't see any LCOW tests changed.

The factor is now part of the unit model's capital_cost variable, as opposed to being applied at the global level.

Ok I see.

Explanation for myself and anyone else: factor_capital_cost was set to 1 so mooted the influence of having TIC applied at the unit model level. So the value of aggregate_capital_cost, which is the sum of unit model capital_cost, didn't change; aggregate_capital_cost is used in total_capital_cost, which is used in LCOW.

For others, this change is being made to create uniformity around "where" the indirect capital cost is applied.

Right now the ZOCostingPackage does things at the unit-level, and the WaterTAPCosting package at the global-level. The PR modifies all the existing detailed models to put it at the unit-level.

bknueven · 2023-11-10T23:01:12Z

@kurbansitterley I believe I have addressed your concerns around the ion exchange model in 8264aae.

kurbansitterley

LGTM

bknueven · 2023-11-10T23:09:10Z

There are two costing PRs open, #1186 and #1196. Should this wait on those?

adam-a-a · 2023-11-11T03:50:04Z

There are two costing PRs open, #1186 and #1196. Should this wait on those?

Not necessarily.

adam-a-a · 2023-11-11T03:53:31Z

watertap/examples/flowsheets/lsrro/tests/test_lssro_paper_analysis.py

+            B_max=None,
+            number_of_RO_finite_elements=10
+        )
+    except ValueError:


Why was this needed? To sidestep the occasional fail related to this test or something else?

If Ipopt terminates with a weird termination condition, then Pyomo will raise a ValueError trying to load the results back into the model.

This simply catches this mode of failure. Note that if the solve fails (or succeeds) and we didn't expect it to the test will still fail.

bknueven · 2023-11-13T21:00:38Z

@kurbansitterley could you have a look at 55e1754? I left a few things out on the previous commit...

kurbansitterley · 2023-11-14T18:33:59Z

@kurbansitterley could you have a look at 55e1754? I left a few things out on the previous commit...

Yes, see that. Most important change was addressed (not double counting indirect costs) and is now reflected in the tests, it appears. So my approval remains!

bknueven · 2023-12-06T16:23:06Z

This is currently held up by the failing notebook test tutorials/nawi_spring_meeting2023.ipynb.

@adam-a-a @avdudchenko any thoughts?

bknueven · 2023-12-07T19:22:28Z

watertap/unit_models/tests/test_cstr.py

+        m.fs.costing.initialize()
+        m.objective = Objective(expr=m.fs.costing.LCOW)
        assert_units_consistent(m)
-        results = solver.solve(m)
+        results = solver.solve(m, tee=True)

        assert_optimal_termination(results)

        # Check solutions
        assert pytest.approx(526.45 * 2, rel=1e-5) == value(
            m.fs.unit.costing.capital_cost
        )
-        assert pytest.approx(1.07948e-5, rel=1e-5) == value(m.fs.costing.LCOW)
+        assert pytest.approx(8.94365e-07, rel=1e-5) == value(m.fs.costing.LCOW)


@MarcusHolly can you have a look at this? It seems like this flowsheet had degrees of freedom but no objective, and therefore the LCOW could converge to an arbitrary value.

I've fixed it here to what I got out of the optimization.

Sure, I can take a look at it soon, just not right now...

Expression

bknueven · 2023-12-07T20:14:48Z

@adam-a-a should this wait on #1213? 7d33365 might be of help with that, so we might want to merge this anyways.

adam-a-a · 2023-12-07T20:15:00Z

This is currently held up by the failing notebook test tutorials/nawi_spring_meeting2023.ipynb.

@adam-a-a @avdudchenko any thoughts?

Is this still an issue or have your latest commits resolved this?

adam-a-a · 2023-12-07T20:18:01Z

@adam-a-a should this wait on #1213? 7d33365 might be of help with that, so we might want to merge this anyways.

I suppose this doesn't need to wait on #1213 , assuming it won't break the BSM2 flowsheet.

bknueven · 2023-12-07T20:18:04Z

This is currently held up by the failing notebook test tutorials/nawi_spring_meeting2023.ipynb.
@adam-a-a @avdudchenko any thoughts?

Is this still an issue or have your latest commits resolved this?

No, that was fixed by 7d33365 (making LCOW an Expression)

bknueven · 2023-12-07T20:20:27Z

@adam-a-a should this wait on #1213? 7d33365 might be of help with that, so we might want to merge this anyways.

I suppose this doesn't need to wait on #1213 , assuming it won't break the BSM2 flowsheet.

Sounds like we should wait ... I mean, it shouldn't, but I am not extraordinarily confident either.

bknueven · 2023-12-07T20:21:18Z

Going to wait on #1213.

…TIC` (watertap-org#1175)" This reverts commit 094da03

bknueven requested review from adam-a-a, TimBartholomew, avdudchenko and kurbansitterley October 19, 2023 02:20

bknueven added the costing label Oct 19, 2023

bknueven mentioned this pull request Oct 19, 2023

Costing Unification: parameter consistency across costing packages #1174

Merged

ksbeattie added the Priority:Normal Normal Priority Issue or PR label Oct 19, 2023

ksbeattie assigned bknueven Oct 19, 2023

avdudchenko approved these changes Oct 24, 2023

View reviewed changes

bknueven force-pushed the factor_total_investment_to_TIC branch 2 times, most recently from d8e8656 to 73878be Compare October 27, 2023 15:53

kurbansitterley reviewed Nov 9, 2023

View reviewed changes

kurbansitterley requested changes Nov 9, 2023

View reviewed changes

bknueven force-pushed the factor_total_investment_to_TIC branch from 73878be to 8264aae Compare November 10, 2023 23:00

kurbansitterley approved these changes Nov 10, 2023

View reviewed changes

adam-a-a reviewed Nov 11, 2023

View reviewed changes

bknueven force-pushed the factor_total_investment_to_TIC branch 2 times, most recently from 51d6b60 to 55e1754 Compare November 13, 2023 16:55

bknueven force-pushed the factor_total_investment_to_TIC branch from 46748f7 to 4cc952e Compare November 14, 2023 16:18

bknueven force-pushed the factor_total_investment_to_TIC branch 4 times, most recently from 060d40e to 6dca9cb Compare November 20, 2023 21:24

bknueven force-pushed the factor_total_investment_to_TIC branch from 6dca9cb to a03385c Compare November 22, 2023 03:30

kurbansitterley mentioned this pull request Nov 29, 2023

IonExchange0D Unit Model and Costing Improvements #1139

Merged

bknueven force-pushed the factor_total_investment_to_TIC branch from a7f7c11 to 1c50315 Compare December 7, 2023 18:28

bknueven added 2 commits December 7, 2023 12:17

resolving TIC / factor_total_investment difference

52b4414

fixing idiosyncrasy with caoh2 unit cost

5103cdd

bknueven force-pushed the factor_total_investment_to_TIC branch from 1c50315 to 2825670 Compare December 7, 2023 19:19

bknueven commented Dec 7, 2023

View reviewed changes

bknueven and others added 8 commits December 7, 2023 12:39

standardizing cost_factor; adding aggregate_direct_captial_cost

1d41fd4

Expression

fix CSTR costing test

fad84c7

make test_lssro_paper_analysis more robust when model is infeasible

4b58619

fix bad oaro_multi test

009d5f1

reconciling ion exchange costing

8370f89

setting custom options to stabilize flowsheet_softening_two_stage

900c1c0

make LCOW an Expression

7d33365

fixing issue with multiple choice costing block

8b34ef1

bknueven force-pushed the factor_total_investment_to_TIC branch from 2825670 to 8b34ef1 Compare December 7, 2023 19:39

bknueven marked this pull request as draft December 7, 2023 20:20

ksbeattie marked this pull request as ready for review December 7, 2023 21:29

ksbeattie merged commit 094da03 into watertap-org:main Dec 7, 2023
24 checks passed

hunterbarber added a commit to hunterbarber/watertap that referenced this pull request Dec 8, 2023

Revert "Costing Unification: move from factor_total_investment to `…

b0adce8

…TIC` (watertap-org#1175)" This reverts commit 094da03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Costing Unification: move from `factor_total_investment` to `TIC` #1175

Costing Unification: move from `factor_total_investment` to `TIC` #1175

bknueven commented Oct 19, 2023

codecov bot commented Oct 19, 2023 •

edited

Loading

avdudchenko left a comment

kurbansitterley Nov 9, 2023

kurbansitterley Nov 9, 2023

kurbansitterley Nov 9, 2023

bknueven Nov 9, 2023

kurbansitterley left a comment

kurbansitterley Nov 9, 2023

kurbansitterley Nov 9, 2023

bknueven Nov 9, 2023

kurbansitterley Nov 9, 2023

bknueven Nov 9, 2023

bknueven commented Nov 10, 2023

kurbansitterley left a comment

bknueven commented Nov 10, 2023

adam-a-a commented Nov 11, 2023

adam-a-a Nov 11, 2023

bknueven Nov 13, 2023

bknueven commented Nov 13, 2023

kurbansitterley commented Nov 14, 2023

bknueven commented Dec 6, 2023

bknueven Dec 7, 2023

MarcusHolly Dec 7, 2023

bknueven commented Dec 7, 2023

adam-a-a commented Dec 7, 2023

adam-a-a commented Dec 7, 2023

bknueven commented Dec 7, 2023

bknueven commented Dec 7, 2023

bknueven commented Dec 7, 2023

	blk.costing_package.add_cost_factor(blk, "TIC")
	blk.costing_package.add_cost_factor(blk, None)

Costing Unification: move from factor_total_investment to TIC #1175

Costing Unification: move from factor_total_investment to TIC #1175

Conversation

bknueven commented Oct 19, 2023

Fixes/Resolves: Part 1 of 3 for #1134

Summary/Motivation:

Changes proposed in this PR:

Legal Acknowledgement

codecov bot commented Oct 19, 2023 • edited Loading

Codecov Report

avdudchenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kurbansitterley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bknueven commented Nov 10, 2023

kurbansitterley left a comment

Choose a reason for hiding this comment

bknueven commented Nov 10, 2023

adam-a-a commented Nov 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bknueven commented Nov 13, 2023

kurbansitterley commented Nov 14, 2023

bknueven commented Dec 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bknueven commented Dec 7, 2023

adam-a-a commented Dec 7, 2023

adam-a-a commented Dec 7, 2023

bknueven commented Dec 7, 2023

bknueven commented Dec 7, 2023

bknueven commented Dec 7, 2023

Costing Unification: move from `factor_total_investment` to `TIC` #1175

Costing Unification: move from `factor_total_investment` to `TIC` #1175

codecov bot commented Oct 19, 2023 •

edited

Loading