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

Solve ASM2D Flowsheet without bound_push and autoscale function #1133

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

adam-a-a
Copy link
Contributor

@adam-a-a adam-a-a commented Sep 13, 2023

Fixes/Resolves:

Summary/Motivation:

An existing flowsheet example for the conventional ASM2D model uses bound_push and an autoscale function. Neither should be used to solve the model in the main codebase. Moreover, the modified version of ASM2D, a trickier variant of conventional ASM2D, will subsequently be implemented in a similar flowsheet example, but we should make sure the original version works first.

TODO:

  • Adjust regression test values and review to make sure we accept the variations in results

Changes proposed in this PR:

  • remove bound_push
  • remove autoscaling

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

@adam-a-a
Copy link
Contributor Author

Note: initialization and solve was working last I checked. All I need to do is update regression test values accordingly since they were apparently sensitive to the change in scaling.

@adam-a-a adam-a-a added iedo Priority:Normal Normal Priority Issue or PR labels Sep 13, 2023
Copy link
Contributor

@bknueven bknueven 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 my meta-comment is this could be okay -- though it would be better to run calculate_scaling_factors after the unit model is initialized instead. Sometimes it is difficult to know good scaling factors without a solution in hand.

If things still "work" at the flowsheet level having a few custom options which just need to be utilized for unit model initialization isn't a huge deal, IMHO, even if it's less than ideal.

@ksbeattie ksbeattie added Priority:Low Low Priority Issue or PR and removed Priority:Normal Normal Priority Issue or PR labels Dec 21, 2023
@ksbeattie ksbeattie added the Backlog Will get to it... someday label Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backlog Will get to it... someday iedo Priority:Low Low Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants