Bring WaterTap in line with changes in IDAES scaling #1695
Conversation
dallan-keylogic
requested review from
MarcusHolly,
Morgan88888888,
adam-a-a,
agarciadiego,
luohezhiming and
sufikaur
as code owners
| # TODO is it intentional that we're setting scaling factors for some constraints | ||
| # but doing constraint scaling transforms for others? |
There was a problem hiding this comment.
It was intentional, but it was long enough ago where I forget the rationale behind it. Likely because I tried constraint scaling transform first and I couldn't get it to solve
There was a problem hiding this comment.
should this comment be removed?
| @linux_platform_only | ||
| def test_condition_number_on_linux(self, system_frame): |
There was a problem hiding this comment.
Seems like this was the "fix" for the condition numbers not matching between Windows and Linux. I think this is fine as we've done similar things in other scaling/condition number tests already
| # Python 3.9-3.11 | ||
| cond == pytest.approx(4.3021828e15, rel=1e-2) | ||
| # Python 3.12 | ||
| or cond == pytest.approx(2.987651e15, rel=1e-2) |
There was a problem hiding this comment.
Hopefully future improvements to scaling will bring these more in line?
| """ | ||
|
|
||
| assert stream.getvalue() == expected | ||
| # TODO Replace these scaling profiler tests with more detailed convergence analysis |
There was a problem hiding this comment.
Perhaps make an issue on this if you haven't already?
| ] == pytest.approx(2617, rel=1e-3) | ||
| ] == pytest.approx(1e4, rel=1e-3) | ||
| assert m.fs.stream[0].scaling_factor[ | ||
| m.fs.stream[0].eq_mass_frac_phase_comp["Liq", "SO4_2-"] | ||
| ] == pytest.approx(468.1, rel=1e-3) | ||
| ] == pytest.approx(1e3, rel=1e-3) | ||
| assert m.fs.stream[0].scaling_factor[ | ||
| m.fs.stream[0].eq_mass_frac_phase_comp["Liq", "Na_+"] | ||
| ] == pytest.approx(89.89, rel=1e-3) | ||
| ] == pytest.approx(1e2, rel=1e-3) | ||
| assert m.fs.stream[0].scaling_factor[ | ||
| m.fs.stream[0].eq_mass_frac_phase_comp["Liq", "Cl_-"] | ||
| ] == pytest.approx(49.74, rel=1e-3) | ||
| ] == pytest.approx(1e2, rel=1e-3) | ||
| assert m.fs.stream[0].scaling_factor[ | ||
| m.fs.stream[0].eq_mass_frac_phase_comp["Liq", "Mg_2+"] | ||
| ] == pytest.approx(717.2, rel=1e-3) | ||
| ] == pytest.approx(1e3, rel=1e-3) | ||
| assert m.fs.stream[0].scaling_factor[ | ||
| m.fs.stream[0].eq_dens_mass_solvent | ||
| ] == pytest.approx(0.001, rel=1e-3) |
There was a problem hiding this comment.
@dallan-keylogic notes that these changes are attributed to "how the nominal value expression walker works. Previously it used 1/value(var) if var was fixed, but now it uses 1/scaling_factor[var] regardless of whether or not var is fixed."
There was a problem hiding this comment.
"One slight correction: it now uses 1/scaling_factor[var] if var has a scaling factor set, but falls back on 1/value(var) if no scaling factor is set, regardless of whether or not var is fixed"
…gic/watertap into new_scaling_changes
Co-authored-by: Ludovico Bianchi <[email protected]>
| # primary requirements for unit and property models | ||
| # allow X.Y.Z stable release(s) and X.Y+1.dev0 (i.e. the main branch after X.Y.Z) | ||
| # disallow X.Y+1.0rc0 (i.e. forcing a manual update to this requirement) | ||
| "idaes-pse >=2.9.0,<2.10.0rc0", |
There was a problem hiding this comment.
changing this to new rc0
7 participants
Fixes/Resolves:
Changes in IDAES caused some test failures in WaterTap (mostly involving scaling). This PR fixes those failures.
This requires IDAES/idaes-pse#1711 from idaes-pse.
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: