Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
base: main
Are you sure you want to change the base?
1D Membrane Model for CO2 Capture and Utilization #1378
Changes from 31 commits
2affa77
b0a336a
775b27a
dd437b7
693fd68
5505b1f
f91e825
446951b
db77a65
508f10c
d31a4cb
e228592
feda25c
054808f
f6dc03b
a6c9697
14d3856
e8bc928
3d8e068
81c3fa3
35a8ee4
53a17b5
97abbe7
b795061
e721692
2cd61a4
3e3dda8
278345b
c30f6f9
0b8a57d
d826433
093de8e
448394f
d6a977c
75cfb24
c6d3677
83529cf
80befee
6f3f8a6
93038d4
d44f95e
410c036
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.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.
Again, why is this disabled? This implies you have an unused import which should be removed.
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 is disabled to pass the pylint test.
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 miss my point. I am asking why you are disabling the test, as you generally should not turn off pytest checks.
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.
To check, does this need to be indexed by time and finite element? Whilst there are cases where you would want this degree of flexibility, is it something you want to support right now?
Also, I do not think this should be indexed by the feed side component list. Either you should have a single property package for the unit model, in which case you should use the uni level package, or it you want to support separate property packages you should use the intersection of the feed and sweep side component lists (i.e. only create a term for those species which appear in both property packages). Otherwise, you run the risk of trying to write constraints involving species which do not exist on the sweep side.
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.
In the simple assumption, it isn't necessary. However, retaining it isn't particularly harmful either.
The example you gave about indexing could occur in some membrane separations but isn't common in this application. I'll keep this idea in mind for future improvements if the need arises.
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 is better to fix these issues now rather than wait until later when you have forgotten how the model works.
You need to make the decision to either use a single property package for everything or not, and if you decide to allow for multiple property packages then you need to go the full way to supporting them.
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.
What happens here if the two property packages have different components? Should this instead use the intersection of the two component lists (which is easy to do)?
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.
Missing space "Unit (GPU)"
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.
fixed
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.
Pylint complains about this - general practice is to do this as single line comments (i.e. use
#
).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.
fixed by using #
Check warning on line 232 in idaes/models_extra/co2_capture_and_utilization/unit_models/membrane_1d.py
GitHub Actions / Pylint
W0105 (pointless-string-statement)
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.
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.
How are you handling the possible over-specification here?
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 looks like it results in an over-specified problem. You write a constraint for
a, b
andb, a
which are reflections of each other. If the user specifies all the selectivities they will have a degree of freedom issue.I think you really need to only cover the unique pairs (and exclude a=b).
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 you needed this earlier. It might also be a good idea to attach this ot the model to save Pyomo replicating this every time you use it.
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.
moved and used for permeance as well
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.
For clarity, I would call this an isothermal constraint. It is not an energy balance.
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.
fixed
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 also suggest changing the name of the constraint (i.e. the name of the rule/function).
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.
changed
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 need logic here to handle cases where the sweep stream is not present.
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.
added