-
Notifications
You must be signed in to change notification settings - Fork 84
Create unit model tools directory and move calculate_operating_pressure
#1699
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
base: main
Are you sure you want to change the base?
Create unit model tools directory and move calculate_operating_pressure
#1699
Conversation
| @@ -0,0 +1,113 @@ | |||
| ################################################################################# | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One note about location--since I noticed the discussion on #1689: the tools folder originally had parameter sweep, in addition to oli_api and some associated oli utilities, the latter of which were then moved to utilities.
To me, this fits more closely with what we have under utilities. The only exception is that this a utility in the context of unit models. I suppose this is subjective, but if you are shooting for consistency alongside historical context, there it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you mean watertap.core.util? I suppose it is subjective and also dependent on what the difference between a "tool" and a "utility" function is.
To me, from watertap.tools.unit_models import ____ (or even from watertap.tools import _____) is more obvious and accessible. I would moderately fight for this hill but not die on it.
| if isinstance(feed_state_block, NaClStateBlockData) or isinstance( | ||
| feed_state_block, NaClTDepStateBlockData | ||
| ): | ||
| comp = "NaCl" | ||
| if isinstance(feed_state_block, SeawaterStateBlockData): | ||
| comp = "TDS" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need these conditionals? Can't we just point to the component on component_list, as we do in unit models? A little complexity arises with MCAS, which we can add logic for, but can happen in a separate PR.
Instead, I'd propose a conditional check on the supported stateblockdata classes and raise an exception if it is not one of the anticipated prop model stateblocks.
|
|
||
| def calculate_operating_pressure( | ||
| feed_state_block=None, | ||
| over_pressure=0.15, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| over_pressure=0.15, | |
| over_pressure_factor=1.15, |
I think it'd be more intuitive for the user if this were a factor representing what factor to multiply the brine osm press by, rather than the fraction that we have now.
Then you'd adjust the equation below and eliminate need to add 1 to the factor.
| tmp = ConcreteModel() # create temporary model | ||
| prop = feed_state_block.config.parameters | ||
|
|
||
| tmp.feed = prop.build_state_block([0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nice. I wonder though if you could just clone the stateblock as your tmp model?
| tmp = ConcreteModel() # create temporary model | |
| prop = feed_state_block.config.parameters | |
| tmp.feed = prop.build_state_block([0]) | |
| tmp = feed_state_block.clone() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm let me try
update: doesn't work.
…mically; check over_pressure_factor >= 1; raise if not optimal termination
|
|
||
| print(over_pressure_factor, water_recovery_mass, salt_passage) | ||
|
|
||
| comp = [c for c in state_block.component_list if c != "H2O"][0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| comp = [c for c in state_block.component_list if c != "H2O"][0] | |
| comp = state_block.params.solute_set |
| "over_pressure_factor argument must be greater than or equal to 1.0" | ||
| ) | ||
|
|
||
| comp = state_block.params.solute_set.first() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming that we are limiting this to single solute prop models right now, I’d recommend just checking the solute_set had a length no greater than 1. If greater than 1, raise exception since not supported yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the state block type check above, is it possible to get to this point with a state block where len(solute_set) > 1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would extend it a bit.
- Check if there is osmotic_pressure available
- Check if there is single solute, if there is more then one, check if there is total_disolved_solids -> this will get MCAS to work although, it needs to have "seawater or NACL" properties for osmotic pressure enabled and density I think as well. This should cover most of our prop packs then.
Fixes/Resolves:
Moves
calculate_operating_pressurehelper function from the RO w ERD flowsheet to a new directory for RO tools.Summary/Motivation:
The
calculate_operating_pressurehelper function in the RO w ERD flowsheet is useful outside of that flowsheet. This PR moves that function to the newly createdwatertap/tools/unit_modelswhere we could put this and other useful tools for specific unit models.calculate_operating_pressurewas modified from the current form to be able to accept:It also provides lower and upper bounds on acceptable arguments for
water_recovery_massandsalt_passage.Changes proposed in this PR:
watertap/toolscalledunit_modelsfor unit model-specific helper functionscalculate_operating_pressurehelper function from the RO w ERD flowsheetLegal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: