-
Notifications
You must be signed in to change notification settings - Fork 228
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
1D Membrane Model for CO2 Capture and Utilization #1378
Conversation
idaes/models_extra/co2_capture_and_utilization/unit_models/README.md
Outdated
Show resolved
Hide resolved
idaes/models_extra/co2_capture_and_utilization/unit_models/membrane_1d.py
Show resolved
Hide resolved
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.
Here is a first batch of comments. Most are minor things and ways to do things better, and otherwise what I have seen so far looks good.
Some general comments are:
- Fix all the pylint issues (either by running
pylint
yourself or looking throguh the notifications here on GitHub). - Fix the tests so that they run.
- We will also need some basic documentation to merge this PR.
idaes/models_extra/co2_capture_and_utilization/unit_models/membrane_1d.py
Show resolved
Hide resolved
idaes/models_extra/co2_capture_and_utilization/unit_models/tests/test_membrane_1d.py
Outdated
Show resolved
Hide resolved
idaes/models_extra/co2_capture_and_utilization/unit_models/membrane_1d.py
Show resolved
Hide resolved
idaes/models_extra/co2_capture_and_utilization/unit_models/membrane_1d.py
Show resolved
Hide resolved
idaes/models_extra/co2_capture_and_utilization/unit_models/membrane_1d.py
Outdated
Show resolved
Hide resolved
idaes/models_extra/co2_capture_and_utilization/unit_models/membrane_1d.py
Show resolved
Hide resolved
idaes/models_extra/co2_capture_and_utilization/unit_models/membrane_1d.py
Outdated
Show resolved
Hide resolved
idaes/models_extra/co2_capture_and_utilization/unit_models/membrane_1d.py
Outdated
Show resolved
Hide resolved
idaes/models_extra/co2_capture_and_utilization/unit_models/membrane_1d.py
Show resolved
Hide resolved
idaes/models_extra/co2_capture_and_utilization/unit_models/membrane_1d.py
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1378 +/- ##
==========================================
+ Coverage 77.60% 77.63% +0.02%
==========================================
Files 391 393 +2
Lines 64333 64462 +129
Branches 14245 14272 +27
==========================================
+ Hits 49926 50042 +116
- Misses 11831 11841 +10
- Partials 2576 2579 +3 ☔ View full report in Codecov by Sentry. |
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.
A number of comments. A few other things:
- There are a lot of lines of code that are not covered by tests (mostly things like different combinations of configurations). Whilst not required for models_extra, I would strongly encourage you to do at least minimal testing of the different configurations to ensure they at least build successfully.
- You do not have the methods required to support the
report
and stream table functionality (and thus the visualizer). Whilst again not critical, they would be a good thing to include for usability. - You need to include some basic documentation. At a minimum of explaining what these models are for, the key equations and what degrees of freedom the user should set (this is especially important given my comment about selectivity).
One-dimensional membrane class for CO2 gas separation | ||
""" | ||
|
||
# pylint: disable=unused-import |
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 is this being disabled? Enum
is being used, so there should not be a warning issued by this.
idaes/models_extra/co2_capture_and_utilization/unit_models/membrane_1d.py
Show resolved
Hide resolved
idaes/models_extra/co2_capture_and_utilization/unit_models/membrane_1d.py
Show resolved
Hide resolved
idaes/models_extra/co2_capture_and_utilization/unit_models/membrane_1d.py
Show resolved
Hide resolved
self.cell_length = Expression(expr=self.length / self.config.finite_elements) | ||
|
||
self.cell_area = Var( | ||
initialize=100, units=units.cm**2, doc="The membrane area" |
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 needs a better doc string, as at the moment it is the same as self.area
. I think this should be area per cell (or finite element to be more accurate).
self.selectivity = Var( | ||
self.flowsheet().time, | ||
self.mscontactor.elements, | ||
self.mscontactor.feed_side.component_list, |
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 need the full factorial combination of these? I suspect you do not need the combination where both components are the same, and that S[i, j] == 1/S[j, i]
. Looking below, I think this is confirmed by the following constraint.
Thus, my question becomes how do you intend for a user to use these? If a user were to naively fix all the selectivity, then the problem would be over-specified.
mb_units = feed_side_units.FLOW_MASS | ||
rho = self.mscontactor.feed_side[t, s].dens_mass | ||
else: | ||
raise TypeError("Undefined flow basis, please define the flow basis") |
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.
Minor comment, but the flow basis is always defined, but we allow for it to be "other" (e.g. volume). A more correct error message would be "This model only supports MaterialFlowBasis equal to molar or mass".
assert m.fs.unit.config.property_package is m.fs.properties | ||
|
||
|
||
class TestMembrane(object): |
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.
You do not need the (object)
when declaring a class
in Python 3.
initializer.initialize(membrane.fs.unit) | ||
results = solver.solve(membrane) | ||
# Check for optimal solution | ||
assert check_optimal_termination(results) |
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 should be assert_optimal_termination
(it combines the assert with check_optimal_termination
for you).
@pytest.mark.solver | ||
@pytest.mark.skipif(solver is None, reason="Solver not available") | ||
@pytest.mark.component | ||
def test_enthalpy_balance(self, membrane): |
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.
You should also check for material conservation here.
Also, Sphinx is complaining about a doc string in you model. I think you need to add a line break at the end of line 57 in you model. |
@Morgan88888888 any update on this? |
I am working on it and should be ready this week. |
What about this week? We plan on cutting the May release at the end of the month. |
Pushing this to August. |
@Morgan88888888 any news? We're now on the Aug release cycle. |
# Framework (IDAES IP) was produced under the DOE Institute for the | ||
# Design of Advanced Energy Systems (IDAES). | ||
# | ||
# Copyright (c) 2018-2023 by the software owners: The Regents of the |
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.
Copyright year range should be: 2018-2024
# Framework (IDAES IP) was produced under the DOE Institute for the | ||
# Design of Advanced Energy Systems (IDAES). | ||
# | ||
# Copyright (c) 2018-2023 by the software owners: The Regents of the |
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.
Copyright year range should be: 2018-2024
# Framework (IDAES IP) was produced under the DOE Institute for the | ||
# Design of Advanced Energy Systems (IDAES). | ||
# | ||
# Copyright (c) 2018-2023 by the software owners: The Regents of the |
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.
Copyright year range should be: 2018-2024
@Morgan88888888 what's the news on this? |
@Morgan88888888, we're closing this as it appears stalled. Feel free to re-open if/when you are able to work on it again. |
Summary/Motivation:
There is a new project ongoing to support NETL's reactive capture technology. The goal of this work is to integrate CO2-selective polymer membranes with electrochemical conversion to efficiently transform captured CO2 into formic acid. The unit and flowsheet models developed in the field of capturing and utilizing CO2 during this project will be maintained in this repository. These models will benefit other ongoing work, such as membrane design and process configuration optimization.
Changes proposed in this PR:
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: