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

Generic property package fixes #16

Open
wants to merge 28 commits into
base: main
Choose a base branch
from
Open

Generic property package fixes #16

wants to merge 28 commits into from

Conversation

alma-walmsley
Copy link
Collaborator

@alma-walmsley alma-walmsley commented Dec 24, 2024

Generic property package fixes

  • Switch back to the FTPx state definition

    • On closer inspection, FPhx is just an extension of FTPx by providing enth_mol as a variable. It has the exact same variables and constraints when built, except for an extra variable and constraint in FPhx. This means FPhx will not behave any better than FTPx.
    • Being in FPhx, this increases the complexity of the model for no good reason (+1 variable and constraint). If we want to fix enth_mol, adding a constraint for the expression in FTPx has the same effect.
    • In FPhx, We also have to worry about state bounds for enthalpy.
    • There are no separate initialization methods; they are all built around T and P (recall: T is still a variable in this state definition). But maybe this is not a bad thing? Since we want to be able to use temperature anyway.
  • Increase ITER_MAX for estimating temperature_bubble and temperature_dew

  • Fix solving with constraints instead of state variables

    • We can set the state_vars_fixed kwarg in the generic property initialization routine to True to indicate that all state variables are fixed, so it won't try to fix them, and instead just does a degrees-of-freedom check.
    • Still faced some issues with deactivation and re-activation of constraints during different steps of the initialization routine (ie. our constraint to define a state variable gets deactivated and isn't activated until later, but it's needed earlier for 0 degrees of freedom). Ended up modifying the initialization routine to allow this, and created a PR to idaes: Generic property package: Using constraints to define state variables IDAES/idaes-pse#1554. I do currently have issues with not fixing pressure (ie. using enth_mol / temperature to define pressure), so maybe this requires some more thought.
    • Extended the fix_state_vars method from idaes.core.util.initialization in the modular property package for handling constraints. We should probably look at doing this for the helmholtz property package as well, since it takes in state_args which are guesses for unfixed variables (provided from control volume initialization). We have probably just been lucky that helmholtz is pretty simple.
    • Added a few different tests for these.
  • Add a return value for the constrain method in modular and helmholtz property package

@alma-walmsley alma-walmsley marked this pull request as draft December 24, 2024 05:01
@alma-walmsley alma-walmsley marked this pull request as ready for review December 24, 2024 07:38
@alma-walmsley
Copy link
Collaborator Author

alma-walmsley commented Dec 24, 2024

Just a heads up @Mahaki-Leach @bertkdowns you can get some nasty infeasible solves if mole_frac_comp does not sum exactly to 1. At T=~403, P=200000, having 0.33 of argon, oxygen, and nitrogen (sum: 0.99) created this flat line in the T - h curve: It is not a small range... 🙂

sum(mole_frac_comp) == 0.99

image

sum(mole_frac_comp) == 1

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants