Skip to content

Liquid liquid extractor #91

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.

Sign up for GitHub

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

Jump to bottom
Merged
andrewlee94 merged 26 commits into from
May 30, 2024
Merged

Liquid liquid extractor #91

andrewlee94 merged 26 commits into from
May 30, 2024

Conversation

@JavalVyas2000
Copy link
Contributor

@JavalVyas2000 JavalVyas2000 commented Mar 13, 2024 •
edited by github-actions bot
Loading

Liquid liquid extractor example

This PR is a comprehensive tutorial on how to create a unit model from scratch, including a property package and unit model. It also includes how to write the tests for both. This unit model also tackles two phases and has two inlet and outlet ports, thereby giving an example for the user to create a new unit model that has multiple phases with multiple ports for inlet and outlet.

Legal 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.txt 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.

📚 Documentation preview 📚: https://idaes-examples--91.org.readthedocs.build/en/91/

andrewlee94
andrewlee94 suggested changes Mar 13, 2024
View reviewed changes
Copy link
Contributor

@andrewlee94 andrewlee94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a number of specific comments, but an overall question is whether this should be done using the MSContactor model instead of 2 control volumes.

@agarciadiego
Copy link
Contributor

agarciadiego commented Mar 13, 2024

General comments:

  • Add more links to other examples/where you would find packages/documentation for individual components of the property/model/flowsheet building
  • Add diagnostics tool box and add links

@ksbeattie ksbeattie added the Priority:Normal Normal Priority Issue or PR label Mar 14, 2024
@ksbeattie ksbeattie mentioned this pull request Mar 14, 2024
Closed
@bpaul4
Copy link
Contributor

bpaul4 commented Apr 8, 2024

Hi @JavalVyas2000, I can review this PR today if it's ready. The branch is significantly behind main, is it safe to update the branch?

I'm curious why test_browse.py was deleted.

@JavalVyas2000
Copy link
Contributor Author

JavalVyas2000 commented Apr 8, 2024

Hi @bpaul4, the branch is ready to be reviewed. The reason why test_browse.py was deleted becasue the CI has issues with PySimpleGUI and it was failing tests. I just wanted to make sure that the integration was fine and passed all the tests. I believe we can update the branch.

bpaul4
bpaul4 reviewed Apr 8, 2024
View reviewed changes
Copy link
Contributor

@bpaul4 bpaul4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nicely done! The property packages and notebook files are very comprehensive.

I have a number of comments, mostly minor suggestions to improve the readability of the code and notebook. Also, in general when referencing other code features in our toolsets it's most helpful to link to documentation or notebook examples if they exist, and Python code files if more readable sources are not available.

idaes_examples/notebooks/docs/unit_models/custom_unit_models/creating_unit_model.ipynb
"------------------------------------------------------------------------------------\n",
"Suggested next steps:\n",
"\n",
" Try to initialize/solve your model and then call report_numerical_issues()\n",
Copy link
Contributor

@bpaul4 bpaul4 Apr 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, only include solver output from after the diagnostics tips are fixed so that there is only one, clean solver output. The goal of this notebook is to demonstrate creating a custom unit model, not the diagnostics tool itself. If we have an example of using the diagnostics, you can add a link to that notebook.

andrewlee94
andrewlee94 suggested changes Apr 9, 2024
View reviewed changes
Copy link
Contributor

@andrewlee94 andrewlee94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some additional to @bpaul4's comments plus a number of other requests (pretty much all are minor implementation issues rather than modeling issues).

@JavalVyas2000 JavalVyas2000 requested a review from bpaul4 April 18, 2024 18:44
@ksbeattie
Copy link
Member

ksbeattie commented May 9, 2024

@JavalVyas2000 any news here?

andrewlee94
andrewlee94 suggested changes May 13, 2024
View reviewed changes
Copy link
Contributor

@andrewlee94 andrewlee94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more things to fix. Some general questions/issues:

  1. I did a very quick skim of the notebooks and I did not see any test cells in them. Whilst you do have a section on how to write tests for a model, I do not think you have any cell that actually calls these tests to check that they (and the model code) runs as expected.
  2. Do we need the separate copies of the various modules? The issue with these is that they are currently untested as well, and that by effectively duplicating the code in the notebooks you open the door for them to drift out of sync.

idaes_examples/tests/test_browse.py
from idaes_examples import browse


@pytest.mark.unit
Copy link
Contributor

@andrewlee94 andrewlee94 May 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure about how the testing of the notebooks is done, but is this test module necessary?

Copy link
Contributor Author

@JavalVyas2000 JavalVyas2000 May 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lbianchi-lbl could you please help with this question.

pyproject.toml Outdated
keywords = ["IDAES", "energy systems", "chemical engineering", "process modeling"]

[project.optional-dependencies]
gui = [
Copy link
Contributor

@andrewlee94 andrewlee94 May 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this appearing in this PR?

MarcusHolly
MarcusHolly reviewed May 23, 2024
View reviewed changes
Copy link
Contributor

@MarcusHolly MarcusHolly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of minor comments and suggestions

idaes_examples/notebooks/docs/unit_models/custom_unit_models/creating_unit_model.ipynb Outdated
"# Creating Custom Unit Model\n",
"Author: Javal Vyas \n",
"Maintainer: Javal Vyas \n",
"Updated: 2023-02-20\n",
Copy link
Contributor

@MarcusHolly MarcusHolly May 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be updated?

Copy link
Contributor Author

@JavalVyas2000 JavalVyas2000 May 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this does not need to be updated. I should remove the updated field.

idaes_examples/notebooks/docs/unit_models/custom_unit_models/creating_unit_model.ipynb
"- Steady-state only\n",
"- Organic phase property package has a single phase named Org\n",
"- Aqueous phase property package has a single phase named Aq\n",
"- Organic and Aqueous phase properties need not have the same component list. \n",
Copy link
Contributor

@MarcusHolly MarcusHolly May 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to mention the caveat (here and in the unit model file) that they should share at least one component?

MarcusHolly
MarcusHolly approved these changes May 30, 2024
View reviewed changes
Copy link
Contributor

@MarcusHolly MarcusHolly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - one more minor suggestion

agarciadiego
agarciadiego approved these changes May 30, 2024
View reviewed changes
Copy link
Contributor

@agarciadiego agarciadiego left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@andrewlee94 andrewlee94 enabled auto-merge (squash) May 30, 2024 19:25
@andrewlee94 andrewlee94 merged commit 3d3723f into IDAES:main May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@agarciadiego agarciadiego agarciadiego approved these changes

@MarcusHolly MarcusHolly MarcusHolly approved these changes

@lbianchi-lbl lbianchi-lbl Awaiting requested review from lbianchi-lbl

@bpaul4 bpaul4 Awaiting requested review from bpaul4

@dallan-keylogic dallan-keylogic Awaiting requested review from dallan-keylogic

+1 more reviewer

@andrewlee94 andrewlee94 andrewlee94 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@JavalVyas2000 JavalVyas2000

Labels

Priority:Normal Normal Priority Issue or PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@JavalVyas2000 @agarciadiego @bpaul4 @ksbeattie @andrewlee94 @MarcusHolly @lbianchi-lbl