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

Update BSM2-P CSTR Model #1520

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

Conversation

MarcusHolly
Copy link
Contributor

Summary/Motivation:

The BSM2-P flowsheet used the IDAES CSTR, but it should be using the WaterTAP CSTR, which adds a constraint for hydraulic retention time.

Changes proposed in this PR:

  • Substitutes IDAES CSTR for WaterTAP CSTR in BSM2-P flowsheet
  • Updates the flowsheet's scaling factor

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.

@@ -537,6 +537,10 @@ def scale_variables(m):
if "conc_mass_comp" in var.name:
iscale.set_scaling_factor(var, 1e1)

for unit in ("R1", "R2", "R3", "R4"):
block = getattr(m.fs, unit)
iscale.set_scaling_factor(block.hydraulic_retention_time, 1e-3)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are fixing volume, does it make sense to scale this by inverse of volume/flowrate, perhaps using plant influent flowrate as a start?

Copy link
Contributor Author

@MarcusHolly MarcusHolly Nov 7, 2024

Choose a reason for hiding this comment

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

I've just tried doing this, but none of these new scaling factors work (i.e. the system fails to solve)

Copy link
Contributor

Choose a reason for hiding this comment

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

what if we don't provide the scaling facotrs for the HRT as old version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same thing - if we don't provide any scaling factors for HRT, the system fails to solve

Copy link
Contributor

@adam-a-a adam-a-a left a comment

Choose a reason for hiding this comment

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

LGTM- checks are passing so I am assuming these changes to make the model any more unstable than it already is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:Normal Normal Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants