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

Lazily constructing cost attribute blocks #740

Closed
wants to merge 4 commits into from

Conversation

bknueven
Copy link
Contributor

@bknueven bknueven commented Sep 16, 2022

Fixes/Resolves: Towards #676

Summary/Motivation:

The number of typically unused parameters on the WaterTAP Costing block can be confusing to users. This PR would delay construction of those parameters until they are used.

Changes proposed in this PR:

  • Add LazyBlock and LazyBlockMixin to facilitate on-demand block construction
  • Use LazyBlocks for WaterTAPCosting parameters.

Discussion:

  • This may be too clever for our use-case -- the unit model costing methods could just call-out to construct the block themselves
  • Conversely, if we do not implement something like this PR, then any user implementing their own costing method utilizing the global parameters needs to know to call the parameter block construction.
  • We could "meet in the middle" and not have these blocks constructed on access, but instead via some method call.

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.

@codecov
Copy link

codecov bot commented Sep 16, 2022

Codecov Report

Merging #740 (b1ceea9) into main (92f6ab3) will decrease coverage by 0.95%.
The diff coverage is 100.00%.

❗ Current head b1ceea9 differs from pull request most recent head 437e85f. Consider uploading reports for the commit 437e85f to get more accurate results

@@            Coverage Diff             @@
##             main     #740      +/-   ##
==========================================
- Coverage   95.63%   94.67%   -0.96%     
==========================================
  Files         254      221      -33     
  Lines       24342    20767    -3575     
==========================================
- Hits        23280    19662    -3618     
- Misses       1062     1105      +43     
Impacted Files Coverage Δ
watertap/core/__init__.py 100.00% <100.00%> (ø)
watertap/core/lazy_block.py 100.00% <100.00%> (ø)
watertap/costing/watertap_costing_package.py 100.00% <100.00%> (+1.19%) ⬆️
...ies/wastewater_resource_recovery/metab/metab_ui.py 0.00% <0.00%> (-100.00%) ⬇️
...tudies/wastewater_resource_recovery/metab/metab.py 89.03% <0.00%> (-10.53%) ⬇️
watertap/core/zero_order_costing.py 90.93% <0.00%> (-7.80%) ⬇️
watertap/unit_models/uv_aop.py 94.69% <0.00%> (-2.19%) ⬇️
watertap/property_models/ion_DSPMDE_prop_pack.py 91.61% <0.00%> (-0.96%) ⬇️
...heets/full_treatment_train/analysis/multi_sweep.py 79.67% <0.00%> (-0.82%) ⬇️
...trient_removal/electrochemical_nutrient_removal.py 99.04% <0.00%> (-0.22%) ⬇️
... and 112 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 September 16, 2022 22:35
@ksbeattie ksbeattie added the Priority:Normal Normal Priority Issue or PR label Sep 29, 2022
@bknueven bknueven marked this pull request as draft October 13, 2022 21:32
@bknueven
Copy link
Contributor Author

As part of, or maybe before, the way flows cost attributes are created should be reconsidered with IDAES/idaes-pse#983.

@bknueven
Copy link
Contributor Author

bknueven commented Nov 2, 2022

Closing in favor of #835.

@bknueven bknueven closed this Nov 2, 2022
@bknueven bknueven deleted the lazy_costing 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:Normal Normal Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants