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

Enable derived costing package to define a flow cost #983

Merged
merged 3 commits into from
Oct 6, 2022

Conversation

bknueven
Copy link
Contributor

@bknueven bknueven commented Oct 5, 2022

Fixes N/A

Summary/Motivation:

In the WaterTAP costing package, we have names like electricity_base_cost for predefining flow costs. It would be less brittle and error-prone if the WaterTAP costing package could just use the name electricity_cost directly:
https://github.com/watertap-org/watertap/blob/c71b898cf490b7dc6f7a0115e95b8dbb6a771adf/watertap/costing/watertap_costing_package.py#L122-L128. Here, changing this mutable Param doesn't actually do anything, because its value is extracted when the electricity_cost Var is created.

Changes proposed in this PR:

  • When registering a flow, if the name {flow_type}_cost is already on the global costing block, just use it instead of creating a new Var.

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 requested a review from andrewlee94 October 5, 2022 20:34
Copy link
Contributor

@adam-a-a adam-a-a 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
Member

@andrewlee94 andrewlee94 left a comment

Choose a reason for hiding this comment

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

I think there is an edge case that we need to address, plus we should probably note something about this in the docs (docs/reference_guides/core/costing/costing_framework.rst).

current_component = self.component(name)
if current_component is not None and current_component is cost:
pass
else:
Copy link
Member

Choose a reason for hiding this comment

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

I think I see an edge case here that needs to be caught - what happens if a component name already exists but is not cost? I think this would implicitly replace the existing component, so I think we need to catch this and raise an Exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree -- I was just making a minimal change to existing behavior. Happy to add an Exception here though.

Copy link
Member

Choose a reason for hiding this comment

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

I think an Exception is the best approach - it is easy to add in here, and I generally never try to second guess the user. I.e. if they do something wrong it is best to tell them to fix it rather than make assumptions.

@codecov
Copy link

codecov bot commented Oct 5, 2022

Codecov Report

Base: 69.93% // Head: 69.93% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (ff7ca31) compared to base (46fac58).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #983      +/-   ##
==========================================
- Coverage   69.93%   69.93%   -0.01%     
==========================================
  Files         397      397              
  Lines       64922    64927       +5     
  Branches    11849    11851       +2     
==========================================
+ Hits        45406    45408       +2     
- Misses      17259    17263       +4     
+ Partials     2257     2256       -1     
Impacted Files Coverage Δ
idaes/core/base/costing_base.py 89.16% <100.00%> (+0.27%) ⬆️
idaes/ver.py 61.53% <0.00%> (-4.62%) ⬇️

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@bknueven
Copy link
Contributor Author

bknueven commented Oct 6, 2022

I think there is an edge case that we need to address, plus we should probably note something about this in the docs (docs/reference_guides/core/costing/costing_framework.rst).

I've added a RuntimeError (along with a test).

For the documentation, I've opted to extend the docstring for this function. The costing framework documentation doesn't seem to mention flow costing.

Copy link
Member

@andrewlee94 andrewlee94 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.

@ksbeattie ksbeattie added the Priority:Normal Normal Priority Issue or PR label Oct 6, 2022
@andrewlee94 andrewlee94 merged commit d236d5e into IDAES:main Oct 6, 2022
@bknueven bknueven deleted the custom_flow_cost branch October 6, 2022 20:45
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
No open projects
Development

Successfully merging this pull request may close these issues.

4 participants