-
Notifications
You must be signed in to change notification settings - Fork 30
Adds two-salt diafiltration unit model #139
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?
Adds two-salt diafiltration unit model #139
Conversation
pyomo model code originating from branch: mollydougher/prommis/multi-component-models
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #139 +/- ##
==========================================
+ Coverage 97.03% 97.16% +0.13%
==========================================
Files 65 71 +6
Lines 9715 10305 +590
==========================================
+ Hits 9427 10013 +586
- Misses 288 292 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/prommis/nanofiltration/tests/test_diafiltration_two_salt.py
Outdated
Show resolved
Hide resolved
src/prommis/nanofiltration/tests/test_diafiltration_two_salt.py
Outdated
Show resolved
Hide resolved
src/prommis/nanofiltration/tests/test_diafiltration_two_salt.py
Outdated
Show resolved
Hide resolved
…ub.com/mollydougher/prommis into adds-two-salt-diafiltration-unit-model
@bpaul4 @MarcusHolly Besides the unit model documentation (which I am still working on), this is ready for another round of reviews. |
@bpaul4 The combination of using backward-backward discretization, updating the tolerances for values I expect to be zero (making them numerically zero), and setting specific constraints for the fluxes I expect to be zero (at x=0) seems to have resolved the numerical diagnostics issues. |
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.
LGTM
# add the membrane unit model | ||
m.fs.membrane = TwoSaltDiafiltration( | ||
property_package=m.fs.properties, | ||
NFEx=10, | ||
NFEz=5, | ||
) |
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 tried solving the model with different NFEx
and NFEz
values. When changing the NFEz
, the model solves great even with large NFEz
values. However, an error is reported when trying different NFEx
values. The error points to a port mismatch
in the m.fs.retentate_stream
(line 127) and m.fs.permeate_stream
(line 131) arcs due to the flow_vol
port variable having mismatched indices on ports fs.membrane.retentate_outlet
and fs.retentate_block.inlet
, and fs.membrane.permeate_outlet
and fs.permeate_block.inlet
, respectively. As far as I can tell, this is caused by two things:
- The
SoluteProductParameter
block having its variables defined on anx
domain generated within diafiltration_solute_product_properties.py separately and independently from the membrane unit model block (TwoSaltDiafiltration
). So when a user changes theNFEx
value for the membrane model (line 60 in diafiltration_flowsheet_two_salt.py), they also need to navigate to the products properties file and set thenfe
value there (line 53 in diafiltration_solute_product_properties.py). - Even when the
NFEx
and thenfe
values are set to be equal in the two separate files, the indices of the variables of the membrane model and port are mismatched for most number of finite elements values (nfe=10 seems to be a lucky number).
If doable, it would be nice to have the feature of setting the number of finite elements in a single location at the flowsheet level, and have it automatically propagate to the membrane unit model and to the property package for the products. Otherwise, please include a comment instructing potential users to change the number of finite elements in both locations.
Finally, please look into having the membrane unit model and product properties' x
domain indices match for different number of finite element values.
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.
Thank you for your comment @aostace01. I will start with @dallan-keylogic's suggestion to address this.
With regard to nfe=10 being lucky, I believe this is due to the two independent discretizations you mentioned, which use slightly different methods, so there is a significant digit issue between the ports.
Hi, @mollydougher. I found a bug - please see my comment. Everything else looks good to me; the PR can be merged as soon as the bug is addressed. |
Thank you @aostace01 for catching that. However, I disagree about making Typically how we deal with this situation is to not have the property state variables indexed by space and time, but rather create a |
@aostace01 @dallan-keylogic I believe the aforementioned bug has been addressed. Please let me know if you have any remaining comments. |
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.
Something I hadn't noticed until this recent changeset is that you're not putting individual streams for the retentate and permeate, but the entire flow and concentration profile across the entire length of the membrane. Is the plan to stack these membrane models lengthwise? If so, the inlet port should also be indexed by length, and you should then create terminal channel models.
If you're not stacking them lengthwise, you should add a permeate material balance, sum up all the material flows on the permeate side, then add a port with an outlet stream that is not indexed by length.
@dallan-keylogic Thank you for catching this. There may be some discussion further down the line, but currently the intention is to stage this membrane model such that the permeate of the previous membrane is the feed/retentate inlet of the next. The permeate mass balances already existed, but I have updated the Port References to reflect that the permeate and retentate product streams are collected at the end of the membrane module. Please let me know if there are any remaining concerns. |
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.
Alright, looks good. Thank you for making changes.
Summary/Motivation:
This PR adds a unit model and sample flowsheet for diafiltration system with two salts. This multi-component model for diafiltration takes lithium and cobalt as a case study, but there will be generalizations to additional solutes in future PRs.
Changes proposed in this PR:
Limitations of the Model:
Planned Improvements (left for future PRs, not necessarily in this order):
Reviewer's checklist / merge requirements:
main
branch on the PR author's forkLegal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution:
I agree my contributions are submitted under the license terms described in the LICENSE.md file
at the top level of this directory.
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.