Skip to content
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

Reorganzing the WaterTAP Costing Package #835

Merged
merged 35 commits into from
Dec 1, 2022

Conversation

bknueven
Copy link
Contributor

@bknueven bknueven commented Nov 2, 2022

Fixes/Resolves: Part of #676

Summary/Motivation:

The WaterTAP costing package was too big. This breaks out all its costing methods and associated parameters into their own files.

Changes proposed in this PR:

  • Separate out costing methods and associated parameters from the watertap costing package into their own modules
  • Add basic utility costing methods and helpers to watertap/costing/util.py
  • Test associated lazy construction techniques for robustness

Questions:

Legal Acknowledgement

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

  1. I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
  2. 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.

@bknueven bknueven self-assigned this Nov 2, 2022
@bknueven bknueven added the Priority:Normal Normal Priority Issue or PR label Nov 2, 2022
@codecov
Copy link

codecov bot commented Nov 7, 2022

Codecov Report

Merging #835 (c91cc06) into main (0e67111) will increase coverage by 0.01%.
The diff coverage is 98.87%.

@@            Coverage Diff             @@
##             main     #835      +/-   ##
==========================================
+ Coverage   95.44%   95.46%   +0.01%     
==========================================
  Files         261      273      +12     
  Lines       25334    25399      +65     
==========================================
+ Hits        24181    24246      +65     
  Misses       1153     1153              
Impacted Files Coverage Δ
watertap/costing/units/ion_exchange.py 93.82% <93.82%> (ø)
watertap/costing/__init__.py 100.00% <100.00%> (ø)
watertap/costing/units/crystallizer.py 100.00% <100.00%> (ø)
watertap/costing/units/electrodialysis.py 100.00% <100.00%> (ø)
watertap/costing/units/energy_recovery_device.py 100.00% <100.00%> (ø)
watertap/costing/units/gac.py 100.00% <100.00%> (ø)
watertap/costing/units/mixer.py 100.00% <100.00%> (ø)
watertap/costing/units/nanofiltration.py 100.00% <100.00%> (ø)
watertap/costing/units/pressure_exchanger.py 100.00% <100.00%> (ø)
watertap/costing/units/pump.py 100.00% <100.00%> (ø)
... and 7 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@bknueven bknueven marked this pull request as ready for review November 7, 2022 19:33
@bknueven bknueven changed the title Adding package-level costing parameter blocks when a unit is costed Reorganzing the WaterTAP Costing Package Nov 7, 2022
@ksbeattie ksbeattie added Priority:High High Priority Issue or PR and removed Priority:Normal Normal Priority Issue or PR labels Nov 10, 2022
Copy link
Contributor

@TimBartholomew TimBartholomew left a comment

Choose a reason for hiding this comment

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

LGTM - one minor comment

@@ -930,13 +930,13 @@ def obj_rule(m):
results = solver.solve(m, tee=False)
assert_optimal_termination(results)

assert pytest.approx(440372.278, rel=1e-5) == value(
assert pytest.approx(453807.598, rel=1e-5) == value(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are the ion exchange costs changing? Was a bug fixed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, look at f3473c3. There were some uncaught issue with units.

@TimBartholomew
Copy link
Contributor

@OOAmusat @lbibl this seems like a big PR, however, it is essentially moving the costing methods for each unit out of the WaterTAP costing package into a separate file. Could one of you review this, you don't need to go through it line by line. Just see one example of the unit model costing methods now interacts with the costing package

Copy link
Contributor

@OOAmusat OOAmusat left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@lbibl lbibl left a comment

Choose a reason for hiding this comment

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

Looks good and a better organization.

@bknueven bknueven enabled auto-merge (squash) November 30, 2022 23:59
@bknueven bknueven merged commit 1af56e6 into watertap-org:main Dec 1, 2022
lbibl pushed a commit to lbibl/watertap that referenced this pull request Dec 1, 2022
* adding working proof-of-concept

* using  attribute

* better error handling

* better example

* hard coding year

* separating remaining units

* costing flows on costing package

* adding init to new dir

* fixing simple erros

* cleaning up imports

* fixing flowsheets

* a few more imports

* setting up to move flows in with units

* fixing import

* moving non-electricity flow definition

* fixing bug in cost_reverse_osmosis

* fixing costing module tests

* fixing electrodialysis 0D tests

* fixing bug in dye_desalination_ui

* cleanup

* testing _DefinedFlowsDict

* adding test for register_costing_parameter_block

* ensuring unit-level costs are convert to base currency

* doing conversions correctly

* removing base_currency update

* fixing units in ion_exchange

* updating ion exchange baselines

* updating UV AOP baselines

* fixing merge conflict mistake
This was referenced Dec 5, 2022
bknueven pushed a commit that referenced this pull request Dec 14, 2022
* save work

* save unfinished work- nonohmic, diffusion layer in places

* save work

* save work, initial solvable version!

* save work

* membrane diffusion and water osmosis under polarizaiontion refined

* save work, all working property

* first finalized ed1d refinement with tests

* typo correction

* improve fs opt

* test opt bound

* rename i_lim configvalue

* typo fix  and rename set

* all pressure drop dimensionless quantity and friction methods added

* fix impacts on existing ed fs

* fix doc building bug

* test opt in win

* test opt numps solver

* save work, add pressure changes in 1d-cv

* save work, modify init routine

* added and tested cv1d deltaP, addressed comments

* add a configerror test

* fixed a missing sf to improve solving for mumps

* change a var name

* added two additional i_lim methods

* improved model structure and scaling

* Add CITATION file for GitHub

* Fix date released

* Add text to use for reference

* Use entity name for WaterTAP authors

* Isothermal 0DRO (#846)

* changing 0DRO to be isothermal like 1DRO

* updating tests

* updating OARO

* updating RO default configuration yaml

* update seawater RO baselines

* update dye desal baselines

* update NF bypass twostage baseline

* update dye desal baselines; again

* OARO Modeling & Initialization (#841)

* setting constraint scaling transform for small membrane channel constraints

* adjusting RO initialization

* restoring custom interface initialization

* fixing bug in new initializer

* updating test_Pdrop_calculated

* fixing cp modulus guess

* updating testing baseline

* tweaking initialization

* move dens_solvent scaling to base class

* fixing bug with MC initialization

* Revert "tweaking initialization"

This reverts commit e015406.

* updating baselines, again

* removing enthalpy from OARO

* removing unneeded import

* undoing membrane channel constraint scaling

* replacing explicit calls to properties_in/properties_out with 1D versions

* Seawater Ion Generic Documentation (#808)

* Test that each ZO model has a ZO doc

* Adds files that were missed in original commit

* Incorporates Ludovico's suggestions

* Minor changes to address comments

* Excel updatted properly now

* Constructed wetlands rst update

* revert changes made in last commit

* Revert "revert changes made in last commit"

This reverts commit 91711ed.

* hardcoding list place

* Preliminary documentation for seawater_ion_generic

* Cl_1- changed to Cl\_-

* Gets rid of extra properties table

* Update index

* Scaling description revised

* Update prop pack description

* Adds additional properties

* Fixes syntax error

* Fixes another syntax error

* Updates introduction to further explain the connection with IDAES

* Updates introduction to further explain the connection with IDAES

Co-authored-by: Bernard Knueven <Bernard.Knueven@nrel.gov>
Co-authored-by: Keith Beattie <ksbeattie@lbl.gov>

* OARO flowsheet (#829)

* Add gui

* delete redundant GLSD UI code

* Draft PR for OARO flowsheet

* troubleshoot OARO flowsheet

* DOF = 0 resolved; solving flowsheet unsuccessful; issue related to OARO or pump/ERDs

* troubleshooting flowsheet

* modify test file

* half flowsheet

* Revise code

* Revise OARO model

* Add test

* Revise OARO

* testing main for coverage

Co-authored-by: adam-a-a <aatia@keylogic.com>
Co-authored-by: Bernard Knueven <Bernard.Knueven@nrel.gov>

* fixing getting started links (#852)

This addresses #844 by changing them from "latest" to "stable".

I also when doing this changed our ReadTheDocs config so that the default is to point to "stable" (which will be the most recent tagged release) rather than "latest" (which is whatever is on the main branch at the time).

* Remove idaes solver directives (#836)

* removing solver requirement from zero order models

* removing in units and full_treatment_train

* Differential Parameter Sweep Sampling (#806)

* Property Package Constraint Indexing & Utility Function Update (#837)

* Update constraint indexing in water prop pack

* Initial seawater prop pack changes

* Change utility function of seawater prop pack

* Updates indexing and utility functions for prop packs

* Addresses issues with parameters in the utility function

* Makes utility function for transforming property constraints

* Update watertap/core/util/scaling.py

Co-authored-by: bknueven <30801372+bknueven@users.noreply.github.com>

* Update scaling.py

* fixing import

* rebase main, resolve conflicts

* resolve conflicts

* Reorganzing the WaterTAP Costing Package (#835)

* adding working proof-of-concept

* using  attribute

* better error handling

* better example

* hard coding year

* separating remaining units

* costing flows on costing package

* adding init to new dir

* fixing simple erros

* cleaning up imports

* fixing flowsheets

* a few more imports

* setting up to move flows in with units

* fixing import

* moving non-electricity flow definition

* fixing bug in cost_reverse_osmosis

* fixing costing module tests

* fixing electrodialysis 0D tests

* fixing bug in dye_desalination_ui

* cleanup

* testing _DefinedFlowsDict

* adding test for register_costing_parameter_block

* ensuring unit-level costs are convert to base currency

* doing conversions correctly

* removing base_currency update

* fixing units in ion_exchange

* updating ion exchange baselines

* updating UV AOP baselines

* fixing merge conflict mistake

* modify var bound in optmizaiton

* modify var bound in optmizaiton

* modify upper bounds of some vars

* remove upper bounds and format

* Empty commit to trigger CI
@bknueven bknueven deleted the costing_separation branch September 22, 2023 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:High High Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants