Skip to content

Byproduct recovery framework tutorial#141

Merged
dallan-keylogic merged 45 commits intomainfrom
byproduct_recovery_framwork_tutorial
Sep 15, 2025
Merged

Byproduct recovery framework tutorial#141
dallan-keylogic merged 45 commits intomainfrom
byproduct_recovery_framwork_tutorial

Conversation

@Lingyan90
Copy link
Contributor

@Lingyan90 Lingyan90 commented Mar 27, 2025

Addresses Issue:

This tutorial explains how to use the byproduct recovery framework.

Summary/Motivation:

This tutorial demonstrated how to apply the byproduct recovery framework to an elements recovery plants. A case study of lithium-colbalt recovery from diafiltration process is utilized.

Changes proposed in this PR:

  • Added a byproduct recovery framework tutorial file
  • Added the tutorial name in doc\_toc.yml

Reviewer's checklist / merge requirements:

  • The head branch (i.e. the "source" of the changes) is not the main branch on the PR author's fork
  • Documentation
  • Tests
  • Diagnostic tests for models

Legal Acknowledgement

By contributing to this software project, I agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the license terms described in the LICENSE.md file
    at the top level of this directory.

  2. 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.

@Lingyan90 Lingyan90 self-assigned this Mar 27, 2025
@Lingyan90 Lingyan90 requested a review from bpaul4 March 27, 2025 14:36
@codecov
Copy link

codecov bot commented Mar 27, 2025

Codecov Report

❌ Patch coverage is 71.42857% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.20%. Comparing base (f54e2b9) to head (e0ef392).
⚠️ Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
...rommis/uky/costing/determine_byproduct_recovery.py 50.00% 1 Missing ⚠️
...costing/tests/test_determine_byproduct_recovery.py 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #141      +/-   ##
==========================================
- Coverage   97.21%   97.20%   -0.01%     
==========================================
  Files          93       93              
  Lines       12587    12589       +2     
==========================================
+ Hits        12236    12237       +1     
- Misses        351      352       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

Hi @Lingyan90, the content here is good, I have some initial comments.

rename the tutorial file
fix typos
@lbianchi-lbl lbianchi-lbl added documentation Improvements or additions to documentation tutorials labels Mar 31, 2025
@lbianchi-lbl
Copy link
Contributor

This is related to the generally applicable functionality introduced mainly in #123.

@bpaul4 bpaul4 requested a review from Agfritz April 8, 2025 14:08
@ksbeattie ksbeattie added the Priority:Normal Normal Priority Issue or PR label Apr 14, 2025
@bpaul4 bpaul4 requested a review from MAZamarripa April 22, 2025 14:08
@Lingyan90
Copy link
Contributor Author

@MAZamarripa addressed your comments based on our meeting discussion

@lbianchi-lbl
Copy link
Contributor

@MAZamarripa provided review feedback offline and the corresponding changes have already been applied. A second round of review is underway. This should be in the June release.

Also: the RtD check failure is due to this tutorial expecting the Costing tutorial being added in #91 to be already present. It is expected to be resolved by itself once #91 is merged in.

@lbianchi-lbl lbianchi-lbl mentioned this pull request Jun 9, 2025
13 tasks
@lbianchi-lbl
Copy link
Contributor

This is waiting for a last round of feedback/approval by @MAZamarripa. It should be merged once #91 has been merged in (RTD checks are expected to fail until then).

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.

Mostly looks good, although there is a large number of warnings. Is there a way to fix those values to exist in the bounds?

@agarciadiego
Copy link
Contributor

@sufikaur can you take a look into the failing docs test

@bpaul4
Copy link
Contributor

bpaul4 commented Aug 18, 2025

@agarciadiego @sufikaur the docs test failure is a reference to a file added in #91 and should pass once that PR is merged.

@Lingyan90
Copy link
Contributor Author

Mostly looks good, although there is a large number of warnings. Is there a way to fix those values to exist in the bounds?

@mollydougher The warnings mentioned above happens when build the dialfiltration model. What's your thought on this?

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.

I think this is great, thank you @Lingyan90 for your efforts here. My one question is why the "Package Installation" section is at the very end of the notebook, and not somewhere after the introduction?

@Lingyan90
Copy link
Contributor Author

I think this is great, thank you @Lingyan90 for your efforts here. My one question is why the "Package Installation" section is at the very end of the notebook, and not somewhere after the introduction?

@bpaul4 , the "Package Installation" section was at the Step 1 part, @MAZamarripa commented that "Package Installation" was not important or not the major foqus of the tutorial, and suggest that the "Package Installaiton" park could be moved to the end of the tutorial or remove it. So I moved it to the end of the tutorial.

@mollydougher
Copy link
Contributor

Mostly looks good, although there is a large number of warnings. Is there a way to fix those values to exist in the bounds?

@mollydougher The warnings mentioned above happens when build the dialfiltration model. What's your thought on this?

@Lingyan90 @bpaul4 I have 2 notes on this issue:

  1. My current understanding is that these warnings pop up during the initialization of each stage because the material_transfer_term is initialized to 0 in the MSContactor unit model. The warnings are resolved by manually initializing the material_transfer_term for each stage to 1e-05 within diafiltration.py.
  2. However, I am still not clear on why these warnings are being thrown in the first place, because the initialized values for the permeate flow rates are non-zero. For example, the warning reads:
WARNING (W1002): Setting Var 'fs.stage1.permeate[0.0,1].flow_vol' to a numeric
value `0.0` outside the bounds (1e-08, None).

but the value of m.fs.stage1.permeate[0.0,1].flow_vol is not set to 0:

Block fs.stage1.permeate[0.0,1]

  Variables:
    flow_vol : Size=1, Index=None, Units=m**3/h
        Key  : Lower : Value               : Upper : Fixed : Stale : Domain
        None : 1e-08 : 0.15000000000000002 :  None : False :  True :  Reals

@jas-yao
Copy link
Contributor

jas-yao commented Sep 2, 2025

Hi @Lingyan90 @bpaul4 @mollydougher

This issue with a large number of warnings also appears in the diafiltration flowsheet model.
This was handled by setting the material_transfer_terms to values that allow permeate flow to be within its bounds, as @mollydougher suggested. The relevant code is here.

It has been a while so the details are not clear to me anymore, but there were initially no warnings when Andrew wrote the diafiltration tutorial and they started showing up after a later IDAES update.

@bpaul4 bpaul4 requested a review from MarcusHolly September 9, 2025 16:48
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 few minor suggestions

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we expect this to be resolved by now? If not, is there some way we can modify these equations to display correctly? My concern is that the equations will be forgotten and remain like this for a while.

"\n",
"This section gives a brief introduction of how to build upon the pre-defined diafiltration process model. The lithium-cobalt diafiltration process is implemented in the file [diafiltration.py](https://github.com/prommis/prommis/blob/main/src/prommis/nanofiltration/diafiltration.py), available in the PrOMMiS repository. \n",
"\n",
"To construct a process flowsheet using this existing model, users can refer to the detailed walkthrough provided in the [Multi_Stream Contactor Tutorial (Solution)](https://github.com/prommis/prommis/blob/main/docs/tutorials/diafiltration-solution.ipynb). This tutorial outlines major order for building the flowsheet. \n",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"To construct a process flowsheet using this existing model, users can refer to the detailed walkthrough provided in the [Multi_Stream Contactor Tutorial (Solution)](https://github.com/prommis/prommis/blob/main/docs/tutorials/diafiltration-solution.ipynb). This tutorial outlines major order for building the flowsheet. \n",
"To construct a process flowsheet using this existing model, users can refer to the detailed walkthrough provided in the [Multi_Stream Contactor Tutorial (Solution)](https://github.com/prommis/prommis/blob/main/docs/tutorials/diafiltration-solution.ipynb). This tutorial outlines the major steps for building the flowsheet. \n",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the equations was displaying right in local run. It shows differently in the Jupyter platform online, this is more like a platform issue and we've discussed this before, and was decided to let it remain like this for now.

"\n",
"To determine whether lithium and cobalt should be recovered, the user first needs to __build the diafiltration process__ and __access the cost and revenue__.\n",
"\n",
"Refer the process model defined in ``diafiltration.py``. Specifically, call the functions in the order specified within the ``main()`` function. Following this order is crucial because the diafiltration process consists of three membrane stages, and the model is built stage by stage. \n",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Refer the process model defined in ``diafiltration.py``. Specifically, call the functions in the order specified within the ``main()`` function. Following this order is crucial because the diafiltration process consists of three membrane stages, and the model is built stage by stage. \n",
"Refer to the process model defined in ``diafiltration.py``. Specifically, call the functions in the order specified within the ``main()`` function. Following this order is crucial because the diafiltration process consists of three membrane stages, and the model is built stage by stage. \n",

"\n",
"__3.1. Specify Product Prices__\n",
"\n",
"Product prices are defined in [costing_dictionary.py](https://github.com/prommis/workspace/blob/main/prommis_workspace/UKy_flowsheet/costing/costing_dictionaries.py). Users can access the default sale prices via ``load_default_sale_prices()`` function, as shown below:"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Product prices are defined in [costing_dictionary.py](https://github.com/prommis/workspace/blob/main/prommis_workspace/UKy_flowsheet/costing/costing_dictionaries.py). Users can access the default sale prices via ``load_default_sale_prices()`` function, as shown below:"
"Product prices are defined in [costing_dictionaries.py](https://github.com/prommis/workspace/blob/main/prommis_workspace/UKy_flowsheet/costing/costing_dictionaries.py). Users can access the default sale prices via ``load_default_sale_prices()`` function, as shown below:"

"source": [
"## __Package Installation__\n",
"\n",
"As mentioned previously, Pyomo, IDAES, and PrOMMiS packages are necessary to use the framework. If these packages are not installed, this maybe done using the following commands in Anaconda Prompt:"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"As mentioned previously, Pyomo, IDAES, and PrOMMiS packages are necessary to use the framework. If these packages are not installed, this maybe done using the following commands in Anaconda Prompt:"
"As mentioned previously, Pyomo, IDAES, and PrOMMiS packages are necessary to use the framework. If these packages are not installed, this may be done using the following commands in Anaconda Prompt:"

Copy link
Contributor

Choose a reason for hiding this comment

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

"the following commands in Anaconda Prompt" are not being shown here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated the text type, it should show up now.

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.

Seems like you addressed all my comments, so LGTM

@dallan-keylogic dallan-keylogic merged commit a65ead4 into main Sep 15, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation Priority:Normal Normal Priority Issue or PR tutorials

Projects

None yet

Development

Successfully merging this pull request may close these issues.