Reorganize unit model notebooks, minor CSTR updates #80
Conversation
|
@bpaul4 The test failures appear to be due to the hydrocarbon example timing out - whilst this is not something you did in this PR you will need to look into why and get it fixed. I believe @agarciadiego wrote the original notebooks so you might need to work with him. However, the first thing I would try would be to apply the IDAES scaling tools to the problem and see if it runs faster. |
|
@andrewlee94 thank you for the suggestion, I will investigate scaling the state variables. I see this issue began on the main repository GitHub Actions 9-10 days ago ~ any chance a merged PR to idaes-pse could have caused this issue? I see updates to the generic and core property files in IDAES/idaes-pse#588. |
|
The notebook with scaling changes (section 3.1 just before initialization) ran under time on my local machine, but still times out in the most recent GitHub Actions run. Would @agarciadiego be available to discuss changes to the notebooks themselves? As we discussed last week I'm hesitant to arbitrarily raise the test limit above 600 seconds. |
|
@bpaul4 @andrewlee94 FYI, this is also causing the failures in the nightly CI runs. Since this is not related to this PR, I've created a dedicated issue for this: #81. |
|
@bpaul4 yes I could be available to discuss changes on the notebooks |
|
I will discuss offline with @agarciadiego. @andrewlee94, would you prefer moving the HC updates to a separate PR? |
|
@bpaul4 The HC changes could be a separate PR (as they are a separate issue), but this PR will need to wait for that to be done. |
|
@lbianchi-lbl @andrewlee94 the CI jobs are passing with the new HC initialization file and variable scaling and this PR is ready for review if you have time today or tomorrow. Other than some minor changes to the HC and CSTR notebooks, the bulk of the changes are relocating non-reactor notebook files to their own subfolder with no code changes. Thanks! |
There was a problem hiding this comment.
I don't think I've ever been more relieved to see all those little green checkmarks! Thanks @bpaul4 for all the work on the HC solver issues on top of the original PR.
There are a few minor changes (mostly concerning link URLs that I believe were fixed in #76 after this PR was created) and checks (making sure that the new structure is used for build-ci.yml as well), but other than that this looks good to me.
src/notebook_index.ipynb
Outdated
| @@ -163,7 +168,7 @@ | |||
| "<a id='examples.surrmod.alamo'></a>\n", | |||
| "\n", | |||
| "#### ALAMO\n", | |||
| "Automated Learning of Algebraic Models, see https://www.minlp.com/alamo-modeling-tool\n", | |||
| "Automated Learning of Algebraic Models, see https://www.minlp.com/alamo\n", | |||
There was a problem hiding this comment.
I believe this is one of the links fixed by @blnicho in #76, so the URL ending in alamo-modeling-tool is the correct one.
| "Automated Learning of Algebraic Models, see https://www.minlp.com/alamo\n", | |
| "Automated Learning of Algebraic Models, see https://www.minlp.com/alamo-modeling-tool\n", |
There was a problem hiding this comment.
I will fix this link and the ones below.
src/notebook_index.ipynb
Outdated
| @@ -12,10 +12,10 @@ | |||
| "metadata": {}, | |||
| "source": [ | |||
| "## Introduction\n", | |||
| "The [IDAES](https://idaes.org) integrated platform ships with a number of examples which can be run on the user's own computer. This page provides links to these examples and provides some guidance in the order in which to try them.\n", | |||
| "The [IDAES](https://www.idaes.org) integrated platform ships with a number of examples which can be run on the user's own computer. This page provides links to these examples and provides some guidance in the order in which to try them.\n", | |||
There was a problem hiding this comment.
| "The [IDAES](https://www.idaes.org) integrated platform ships with a number of examples which can be run on the user's own computer. This page provides links to these examples and provides some guidance in the order in which to try them.\n", | |
| "The [IDAES](https://idaes.org) integrated platform ships with a number of examples which can be run on the user's own computer. This page provides links to these examples and provides some guidance in the order in which to try them.\n", |
src/notebook_index.ipynb
Outdated
| "\n", | ||
| "The IDAES examples are contained in Jupyter Notebooks. In order to view and use this content, you need to open the files with the Jupyter notebook executable (which may be configured on your system as the default application for files of this type). To get started with Jupyter, please see the [Jupyter website](https://jupyter.org) or jump directly to the [official Jupyter Notebook documentation pages](https://jupyter-notebook.readthedocs.io/en/stable/).\n", | ||
| "In addition to viewing and running the examples interactively on your own computer, you can see fully rendered, static versions of the examples in the online [examples documentation](https://idaes.github.io/examples-pse/latest/) pages. For reference documentation on the IDAES integrated platform, please see the online [IDAES-PSE documentation](https://idaes-pse.readthedocs.io/en/stable/index.html).\n" | ||
| "In addition to viewing and running the examples interactively on your own computer, you can see fully rendered, static versions of the examples in the online [examples documentation](https://IDAES.github.io/examples-pse/) pages. For reference documentation on the IDAES integrated platform, please see the online [IDAES-PSE documentation](https://idaes-pse.readthedocs.io/en/stable/index.html).\n" |
There was a problem hiding this comment.
| "In addition to viewing and running the examples interactively on your own computer, you can see fully rendered, static versions of the examples in the online [examples documentation](https://IDAES.github.io/examples-pse/) pages. For reference documentation on the IDAES integrated platform, please see the online [IDAES-PSE documentation](https://idaes-pse.readthedocs.io/en/stable/index.html).\n" | |
| "In addition to viewing and running the examples interactively on your own computer, you can see fully rendered, static versions of the examples in the online [examples documentation](https://idaes.github.io/examples-pse/latest/) pages. For reference documentation on the IDAES integrated platform, please see the online [IDAES-PSE documentation](https://idaes-pse.readthedocs.io/en/stable/index.html).\n" |
| - source: Examples/UnitModels | ||
| - source: Examples/UnitModels/Operations |
There was a problem hiding this comment.
Has this been updated in build-ci.yml as well? If not, it should.
There was a problem hiding this comment.
It has not. I would recommend we update the developer guide https://github.com/IDAES/examples-pse/blob/main/README-developer.md to make updating both build files more clear.
src/Examples/Advanced/CustomProperties/Hydrocarbon_Processing_example.ipynb
Show resolved
Hide resolved
src/Examples/Advanced/CustomProperties/Hydrocarbon_Processing_example.ipynb
Show resolved
Hide resolved
| "\n", | ||
| "EXIT: Optimal Solution Found.\n", | ||
| "\b" | ||
| "EXIT: Optimal Solution Found.\n" |
There was a problem hiding this comment.
This has nothing to do with the review of the changes, but I just thought it was interesting to look at as a relatively clean diff of the changes from the solver's output's point of view. Aside from this, I'm not sure if there's any useful information to be found by a more expert eye.
There was a problem hiding this comment.
The diff of the solve logs show that the newer version actually solves better than the previous one (which is what we would hope for with the addition of scaling and intermediate log variables).
| 6. If you added a subfolder, add this to the `build.yml` file, so the build process will see it. To update the CI | ||
| jobs, add this to the `build-ci.yml` file as well so the build process will see it during integration testing. | ||
| Otherwise, you should not need to do anything with this file. |
* Reorganize unit model notebooks, minor CSTR updates * Added scaling to fix HC Processing timeout error * New initialization file * Minor fixes to index files
Fixes # .
Proposed changes:
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: