Skip to content
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

SOC Flowsheet #100

Merged
merged 16 commits into from
Apr 24, 2024
Merged

SOC Flowsheet #100

merged 16 commits into from
Apr 24, 2024

Conversation

dallan-keylogic
Copy link
Contributor

@dallan-keylogic dallan-keylogic commented Mar 30, 2024

This pull request adds the SOC flowsheet used in recent NMPC and model degradation papers as an example. A Jupyter notebook takes the user through classical control simulations.


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--100.org.readthedocs.build/en/100/

@dallan-keylogic dallan-keylogic marked this pull request as draft March 30, 2024 20:09
@lbianchi-lbl lbianchi-lbl added the Priority:High High Priority Issue or PR label Apr 4, 2024
@lbianchi-lbl
Copy link
Contributor

To run CI against a version if idaes-pse other than main, in requirements-dev.txt, change

idaes-pse @ git+https://github.com/IDAES/idaes-pse@main

to

idaes-pse @ git+https://github.com/IDAES/idaes-pse@refs/pull/1234/merge

where 1234 is the PR number.

@dallan-keylogic dallan-keylogic marked this pull request as ready for review April 23, 2024 13:05
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.

All four example files execute with no issues on my local machine, and the diagram looks good. There are a few commented lines that should be uncommented or cleaned up.

self.feed_heater.default_initializer(
solver=solver, solver_options=optarg, output_level=outlvl
).initialize(model=self.feed_heater)
# self.sweep_heater.initialize(outlvl=outlvl, solver=solver, optarg=optarg)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these be uncommented, or removed?

self.sweep_exchanger.default_initializer(
solver=solver, solver_options=optarg, output_level=outlvl
).initialize(model=self.sweep_exchanger)
# self.sweep_exchanger.initialize(outlvl=outlvl, solver=solver, optarg=optarg)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question for the three commented lines 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.

Forgot to remove the old versions when updating them to the new initialization format. Will take care of that.

Copy link
Contributor

@AlexNoring AlexNoring left a comment

Choose a reason for hiding this comment

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

The examples are running for me as well. I have a few minor comments on the jupyter notebook, but can't comment at the specific locations since the file is too big.

  1. There is an unused import, # from save_results import save_results
  2. You should suppress missing scaling factor warnings to keep the output clean. The following code should work.
scaling_log = idaeslog.getLogger("idaes.core.util.scaling")
scaling_log.setLevel(idaeslog.ERROR)
  1. Near the end of section 2 there is a sentence that is missing a phrase. "Interior point solvers like IPOPT when unconstrained solutions occur too close to variable bounds."
  2. In section 3 the following text has "an" instead of "and". "Most MVs we want to ramp between the old an new setpoints"

@dallan-keylogic dallan-keylogic enabled auto-merge (squash) April 24, 2024 20:27
@dallan-keylogic dallan-keylogic merged commit b4d95e0 into IDAES:main Apr 24, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:High High Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants