-
Notifications
You must be signed in to change notification settings - Fork 84
Adding method for fitting spec sheets to standard RO model #1689
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?
Conversation
|
|
||
| m.fs.properties = props.NaClParameterBlock() | ||
| # seem feed and ro model | ||
| m.fs.feed = Feed(property_package=m.fs.properties) |
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 not eliminate Feed entirely and only use the RO instance?
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.
It was a more transparent process for me as, working with RO feed block is not quiet as straightforward as you can't access flow_vol via the ro inlet port and need to directly interact with feed side properties, which are also connected to other constraints on the model.
Honestly, I dont think it matters it was just faster for me to setup the feed then spend time debuging the ro feed-side properties block.
The other reason is you get the error below as it expects flow vol and concentration to be unfixed... before initialization, it's simple to fix but I remember there being issues like this.
"idaes.core.util.exceptions.PropertyPackageError:
While initializing fs.RO.feed_side.properties, the degrees of freedom are -2, when zero is required.
Initialization assumes that the state variables should be fixed and that no other variables are fixed.
If other properties have a predetermined value, use the calculate_state method before using initialize to determine the values for the state variables and avoid fixing the property variables."
watertap/flowsheets/specsheet_fitting_tools/ro_module_fitter.py
Outdated
Show resolved
Hide resolved
watertap/flowsheets/specsheet_fitting_tools/ro_module_fitter.py
Outdated
Show resolved
Hide resolved
| # Solve the feed to get mass flows and concentrations through out | ||
| result = solver.solve(m.fs.feed) | ||
| assert_optimal_termination(result) | ||
| propagate_state(m.fs.feed_to_ro) |
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.
no need to propagate without feed
| pressure=150 * pyunits.psi, | ||
| membrane_area=40 * pyunits.m**2, | ||
| channel_height=1e-3 * pyunits.m, | ||
| spacer_porosity=0.95, |
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.
spacer porosity is rarely (if ever) provided. I think 0.85 would be a more reasonable default (where 0.95 is a bit closer to that of an open channel).
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.
Thats just a random default option for real modules I use 0.9, changed it 0.85.
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.
Took a quick first pass at the main utility function. I think this is definitely the type of thing we need to be adding to WaterTAP, and this sort of method in particular has been a long time coming.
I have some initial comments, but the main question I have is--would it be better to add this as a method to the RO unit? What would be the pros and cons? Without having thought it through thoroughly, my initial reaction is that this would be better as a method on the RO class.
Co-authored-by: Adam Atia <[email protected]>
Co-authored-by: Adam Atia <[email protected]>
I removed the feed as per the suggestion. Adding it as an explicit method would add unnecessary support code to RO that will need to make a lot of assumptions. I am not really sure what is the best way to go. I really think we want to first figure out a generic way to store parameters/specs for unit models so we can create a way to generate standard output files and have unit models injest them to fix the operation. It really might be better to create a separate tool that handles model data management
There are a few complications -> not all spec sheets are the same or data is the same, some users will need to modify the fitting scripts, some users might have strong opinions on what parameters to fit - > for example, in RO for pressure drop we can fit spare porosity or friction factor correlation values etc. I really think, we first should build a number of such "fitting" scripts for different unit models, and then see if its possible to create generalized methods and what the requirements would be, as creating a new set of tools or adding more functionality to RO/etc. will potentially drive us into a corner. Although creating a data storage method might be the first step. |
| @@ -0,0 +1,30 @@ | |||
| A: | |||
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 think these yamls should be moved to watertap/data, with a specific subdirectory for membrane_element_specs (or whatever we want to name that subdirectory).
I think I could go either way on making it a class method or standalone - but what are thoughts on putting it in a more general location like |
@kurbansitterley Obviously a general tool would live there, but were you suggesting in the interim moving this code (ro_fitter) to tools? Watertap/tools make sense, but i do not think core/utils is a good place. @adam-a-a I was not planning to store fitted data and let users generate the data by running this code, but I don't see why not. I will update it. |
|
|
||
| # m.fs.feed_to_ro = Arc(source=m.fs.feed.outlet, destination=m.fs.RO.inlet) | ||
|
|
||
| TransformationFactory("network.expand_arcs").apply_to(m) |
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 suppose this isn't needed anymore
Yes my suggestion was more just to move it to somewhere else rather than having it in flowsheets. I am curious if you see a tool like this being part of a future |
@kurbansitterley we are on same page with tool location. For the general tooling it would be a separate tools for wrapping around unit models and configuring them for fitting data I would envision - and then generating standardized data files that could be loaded into the model. The data files could be yaml/json format, or what ever else. I think we can discuss the tool and how its structured in a different place as there are many ways to do it and list of cons/pros for each approach can get a tiny bit long.... |
|
@avdudchenko see #1699 for possible location for this (and other) tools |
Fixes/Resolves:
Currently, we do not provide simple scripts to enable fitting of spec sheet data to our unit models. This creates a folder within flowsheets to store such methods, and kicks it off by adding a script for fitting A/B values to standard RO spec sheets.
Summary/Motivation:
We need to provide a simple way for users to fit model parameters from spec sheets. This creates a simple example for fitting RO module to typical spec sheet information for RO modules. Ideally this will start us on a discussion of what more generalized approach might look like and add other unit models to the list.
Changes proposed in this PR:
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: