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

Add new Complementarity formualtion for VLE with cubic EoSs #1397

Merged
merged 40 commits into from
Jun 18, 2024

Conversation

andrewlee94
Copy link
Member

@andrewlee94 andrewlee94 commented Apr 17, 2024

Replaces #977

Summary/Motivation:

This PR adds an implementation of the improved Smooth VLE formulation originally proposed in #977. This implementation preserves the old implementation for backward compatibility.

Changes proposed in this PR:

  • Infrastructure work to support new formulation, and address some pylint issues in modular properties code.
  • Add new class SmoothVLE2 to implement improved formulation

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.

@andrewlee94 andrewlee94 self-assigned this Apr 17, 2024
@andrewlee94 andrewlee94 added enhancement New feature or request Priority:Normal Normal Priority Issue or PR property packages Issues dealing with properties labels Apr 17, 2024
Copy link
Contributor

@dallan-keylogic dallan-keylogic left a comment

Choose a reason for hiding this comment

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

Here are some comments, it seems like the full PR isn't finished yet.

Comment on lines +306 to +323
def identify_VL_component_list(blk, phase_pair):
"""
Identify liquid and vapor phases and which components are in VL equilibrium

Args:
blk: StateBlock of interest
phase_pair: 2-tuple of Phases in equilibrium

Returns:
Lists of component names for:
* liquid Phase object
* vapor Phase object
* components using Raoult's Law
* components using Henry's Law
* liquid only components,
* vapor only components

"""
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this function handle systems with multiple liquid phases? How does this function handle things with solid phases? Now that it's no longer private, we need to consider these questions.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is only for VLE systems, so is not expected to work in those cases. I have added an error for cases where a non-VL pair is passed as the argument.

Comment on lines +389 to +396
# Use lowest component temperature_crit as starting point
# Starting high and moving down generally works better,
# as it under-predicts next step due to exponential form of
# Psat.
# Subtract 1 to avoid potential singularities at Tcrit
Tbub0 = (
min(blk.params.get_component(j).temperature_crit.value for j in raoult_comps)
- 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we convert 1 from Kelvin to the temperature_units of params?

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case, 1 is just an arbitrary value to move away from the exact critical value as some correlations are singular if T=Tc[j].

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, but in edge cases we could end up with a negative temperature (for example, if the user decided to work with temperature reduced by Tc).

Comment on lines 57 to 61
if l_phase is None or v_phase is None:
raise ConfigurationError(
f"{b.params.name} Generic Property Package phase pair {phase_pair[0]}-{phase_pair[1]} "
"was set to use Smooth VLE formulation, however this is not a vapor-liquid pair."
)
Copy link
Contributor

Choose a reason for hiding this comment

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

This error should probably be thrown by identify_VL_component_list.

Copy link
Member Author

Choose a reason for hiding this comment

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

A catch has been added to the utility method, but this can be retained as well.

Comment on lines 68 to 90
s = Var(
b.params.phase_list,
initialize=0.0,
bounds=(0, None),
doc="Slack variable for equilibrium temperature",
units=pyunits.dimensionless,
)
b.add_component("s" + suffix, s)

# Equilibrium temperature
def rule_teq(b):
if b.params.get_phase(phase_pair[0]).is_vapor_phase():
vapor_phase = phase_pair[0]
liquid_phase = phase_pair[1]
else:
vapor_phase = phase_pair[1]
liquid_phase = phase_pair[0]
return (
b._teq[phase_pair]
- b.temperature
- s[vapor_phase] * t_units
+ s[liquid_phase] * t_units
== 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Why make the slack variable s dimensionless when we're multiplying it by t_units here? That also implies it should be scaled.

Comment on lines 95 to 101
eps = Param(
default=1e-04,
mutable=True,
doc="Smoothing parameter for complementarities",
units=f_units,
)
b.add_component("eps" + suffix, eps)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does eps need to be adjusted by the user?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, users should adjust eps as necessary and this will be in the docs (the old formulation was the same).

Comment on lines 121 to 124
def rule_temperature_slack_complementarity(b, p):
flow_phase = b.flow_mol_phase[p]

return smooth_min(s[p] * f_units, flow_phase, eps) == 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, we're treating s as having different units depending on the context. That's not ideal for scaling.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed - if you have any ideas on how to handle this better it would be appreciated. For now I have just focused on getting the code running.

Copy link
Contributor

Choose a reason for hiding this comment

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

My instinct here is to just give s[p] units of temperature per flowrate, then scale this equation like temperature. However, it needs to be scaled with a "temperature difference" scaling factor, not an "absolute temperature" scaling factor.

Copy link
Member Author

Choose a reason for hiding this comment

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

Based on some testing, scaling of s, g and epsilon depends on the larger of T and flow, thus it is hard to know what to do with these. I am inclined to leave these as unit-less for that reason, and any scaling routine can handle the necessary logic behind the scenes (as I doubt users will care enough to understand what is happening).

@andrewlee94 andrewlee94 changed the title Add new Complementatiry formualtion for VLE with cubic EoSs Add new Complementarity formualtion for VLE with cubic EoSs Apr 19, 2024
Copy link

codecov bot commented Apr 19, 2024

Codecov Report

Attention: Patch coverage is 53.23194% with 123 lines in your changes are missing coverage. Please review.

Project coverage is 77.52%. Comparing base (3a1d54a) to head (7997adf).

Files Patch % Lines
...ies/modular_properties/phase_equil/smooth_VLE_2.py 16.21% 93 Missing ⚠️
...erties/modular_properties/base/generic_property.py 60.60% 16 Missing and 10 partials ⚠️
...dels/properties/modular_properties/base/utility.py 94.02% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1397      +/-   ##
==========================================
- Coverage   77.63%   77.52%   -0.12%     
==========================================
  Files         391      392       +1     
  Lines       64391    64510     +119     
  Branches    14264    14268       +4     
==========================================
+ Hits        49989    50010      +21     
- Misses      11830    11935     +105     
+ Partials     2572     2565       -7     

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

@andrewlee94
Copy link
Member Author

@viiibhav @jghouse88 Not sure if either of you still receive these, but input and review on this PR would be welcome.

@jghouse88
Copy link
Member

@viiibhav @jghouse88 Not sure if either of you still receive these, but input and review on this PR would be welcome.

Will take a look this week if that works. The best test would be to see if the paper examples work with this pr and see if we get similar results.

@andrewlee94
Copy link
Member Author

@jghouse88 I have tested this with the existing tests in IDAES, swapping the new formulation for the old one. All but two tests pass with the new formulation, but the two that fail appear to need further investigation - the temperature sweep test is failing to converge to the correct solution (it gets stuck somewhere).

@andrewlee94 andrewlee94 added the CI:run-integration triggers_workflow: Integration label May 9, 2024
@idaes-build idaes-build removed the CI:run-integration triggers_workflow: Integration label May 9, 2024
@andrewlee94 andrewlee94 marked this pull request as ready for review May 28, 2024 17:53
@ksbeattie ksbeattie removed the request for review from dangunter May 30, 2024 18:45

Each complementarity requires a smoothing parameter, named :math:`\epsilon_T` and :math:`\epsilon_Z` for the temperature and cubic root constraints respectively. Within the IDAES model, these are rendered as ``eps_t_phase1_phase2`` and ``eps_z_phase1_phase2``, where ``phase1`` and ``phase2`` are the names assigned to the liquid and vapor phases in the property package (order will depend on the order these are declared).

The tractability of the VLE problem depends heavily upon the values chosen for :math:`\epsilon_T` and :math:`\epsilon_Z`, with larger values resulting in smoother transitions at the phase boundaries (and thus increased tractability) at the expense of decreased accuracy near these points. It is recommended that users employ a 2-stage approach to solving these problems, starting with a larger value of :math:`\epsilon_T` and :math:`\epsilon_Z` initially to determine which region the solution lies in, followed by a second solve using smaller values to refine the solution.
Copy link
Contributor

Choose a reason for hiding this comment

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

"It is recommended that users employ a 2-stage approach to solving these problems, starting with a larger value of :math:\epsilon_T and :math:\epsilon_Z initially to determine which region the solution lies in, followed by a second solve using smaller values to refine the solution."

How is this going to translate to a typical user of IDAES? Are the smoothing parameters global or stuck on state blocks? How can the user find them?

Copy link
Member Author

Choose a reason for hiding this comment

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

They are local, as they potentially need to vary unit-to-unit. Their names are show in the docs.

Comment on lines +109 to +115
s = Var(
vl_phase_set,
initialize=EPS_INIT,
bounds=(0, None),
doc="Slack variable for equilibrium temperature",
units=t_units,
)
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 have any sort of scaling for these slack variables, or is the intention to let the autoscaler do it? This is a case where the results from the autoscaler is going to be highly dependent on initialization, and since the slack variable will frequently be near zero, you may not get any scaling at all.

Comment on lines +223 to +227
try:
teq_cons = getattr(b, "_teq_constraint" + suffix)
# pylint: disable-next=protected-access
iscale.set_scaling_factor(b._teq[phase_pair], sf_T)
iscale.constraint_scaling_transform(teq_cons, sf_T, overwrite=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we're not scaling the slacks then. If we're giving them temperature units, we might as well scale them like temperature. It would be a good first guess, at any rate, and I think the autoscalers will faceplant.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not making any changes to the legacy scaling methods at this point - this code was just copied over from the old implementation. We can think about what is needed for this in the new tools once we get there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Very well, but I do expect this case will need heuristic scaling.

Copy link
Contributor

@dallan-keylogic dallan-keylogic left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Looks good, just a couple minor questions.

if self.temperature.value is not None:
t_value = value(self.temperature)
else:
t_value = None
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is None, does the model break or is there a catch for this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

This just affects whether we can get initial guesses for Teq. Whether the model will solve or not is a separate issue at this point; the goal here is for the code to work without encountering a terminal error.

"temperature_ref": (298.15, pyunits.K),
"parameter_data": {
"PR_kappa": {
("foo", "bar"): 0.000,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why "foo" and "bar"?

Copy link
Member Author

Choose a reason for hiding this comment

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

m = ConcreteModel()
m.params = DummyParameterBlock(
components={
"a": {},
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these supposed to be empty?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes - this is a unit test. We need to define the components so the code runs but don't care about any actual properties.

@andrewlee94 andrewlee94 added the CI:run-integration triggers_workflow: Integration label Jun 17, 2024
@idaes-build idaes-build removed the CI:run-integration triggers_workflow: Integration label Jun 17, 2024
@andrewlee94 andrewlee94 merged commit a66d4c4 into IDAES:main Jun 18, 2024
69 of 93 checks passed
@andrewlee94 andrewlee94 deleted the smooth_vle2 branch June 18, 2024 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Priority:Normal Normal Priority Issue or PR property packages Issues dealing with properties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants