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

Mea column improvements #1077

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

Conversation

dallan-keylogic
Copy link
Contributor

Summary/Motivation:

Miscellaneous changes/fixes in order to improve scaling and convergence of column models.

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 added bug Something isn't working Priority:High High Priority Issue or PR unit models Issues dealing with the unit model libraries labels Feb 2, 2023
Copy link
Member

@andrewlee94 andrewlee94 left a comment

Choose a reason for hiding this comment

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

A few comments on things that need to be fixed or discussed a bit more.

):
if iscale.get_scaling_factor(v) is None: # don't replace if set
name = v.getname().split("[")[0]
index = v.index()
sf = self.config.parameters.get_default_scaling(name, index)
if sf is not None:
iscale.set_scaling_factor(v, sf)

for con in self.component_data_objects(Constraint, descend_into=False):
# if con.name == 'stripper_section.stripper.liquid_phase.properties[0.0,0.8].appr_to_true_species[Liq,CO2]':
Copy link
Member

Choose a reason for hiding this comment

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

This comment needs to be removed.

index = con.index()
sf = self.config.parameters.get_default_scaling(name, index)
if sf is not None:
iscale.constraint_scaling_transform(con, sf, overwrite=False)
Copy link
Member

Choose a reason for hiding this comment

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

This is a fairly significant change to what this method does - we probably need to discuss the implications of this.

Copy link
Member

Choose a reason for hiding this comment

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

Thinking a bit more (and repeating a comment from Slack for posterity) - I think this is the wrong way to go.

IPOPT should be considered a special case, not the general case, in which case we need to set the scaling suffix for the constraints and then let the user decide what to do with them. IPOPT users can use the constraint_scaling_transformation function if they wish, but the generic workflow would use the Pyomo scaling transformation (which relies on having the suffixes).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing the default status of IPOPT and constraint_scaling_transform in the long run is fine, but it's also a complete revolution in how scaling is treated in IDAES models, which make heavy use of constraint_scaling_transform. If this change is reverted, I'll just as likely use constraint_scaling_transform somewhere else to do the desired scaling: probably in electrolyte_states.py.

Copy link
Member

Choose a reason for hiding this comment

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

It is fine to still use constraint_scaling_transform, but it should not be done in the base classes (i.e. anything in idaes.core.base). Ideally, we would only do this in the model level calculate_scaling_factors method.

The point being this is not something we want baked in deep as it will be harder to change later - ideally we want to limit the use of this to the calculate_scaling_factors methods so that when we make the transition, we only need to change those (or if we are lucky, just switch to new methods and delete those as they are no longer needed).

@@ -321,11 +321,11 @@ def build(self):
flow_basis = self.liquid_phase[t_init].get_material_flow_basis()
if flow_basis == MaterialFlowBasis.molar:
fb = "flow_mole"
elif flow_basis == MaterialFlowBasis.molar:
fb = "flow_mass"
# elif flow_basis == MaterialFlowBasis.mass:
Copy link
Member

Choose a reason for hiding this comment

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

Commented code

@lbianchi-lbl
Copy link
Contributor

This model will eventually (soon) be accompanied by a surrogate model. This PR might or might not be merged in the meantime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Priority:High High Priority Issue or PR unit models Issues dealing with the unit model libraries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants