Revised vapor composition constraints fix ZeroDivisionError when gdp.hull reformulation is applied by tristantc · Pull Request #47 · SECQUOIA/gdplib

tristantc · 2024-08-13T16:31:23Z

This PR fix the ZeroDivisionError when gdp.hull reformulation is applied in the Kaibel instance.

File:gdplib/kaibel/kaibel_solve_gdp.py

    tray_exists[1,1].bottom_vapor_composition[1]
    tray_exists[2,1].feedside_vapor_composition[1]
    tray_exists[3,1].productside_vapor_composition[1]
    tray_exists[4,1].top_vapor_composition[1]

…hull reformulation is applied

ZedongPeng · 2024-08-13T21:59:55Z

As long as there are no errors, this PR is ready to be merged. @bernalde

bernalde

Besides some small changes the most important part is verifying that both the Big-M and HR can provided the right solution to the problem and that the new introduced variables/constraints did not modify the optimal solution

bernalde · 2024-08-14T01:34:23Z

        m.dHvap[comp] = dHvapb[comp] / m.Hscale

-    ## Heat capacity calculation for liquid and vapor phases using Ruczika-D method for each component in the feed, section, and tray
+    ## Heat capacity calculation for liquid and vapor phases for each component in the feed, section, and tray


Why remove the comment on the method? Is that method not used? If it is used, leave the comment and add a reference

Addressed in a70395b by restoring the Ruczika-D method comments on the Kaibel heat-capacity and enthalpy calculations.

Addressed in 90e289d. The method comments are kept, corrected to Ruzicka-Domalski, and the README now includes the source reference.

bernalde · 2024-08-14T01:34:55Z

+            m.prop[comp, 'TC'] / m.Tup,
+            m.prop[comp, 'TC'] / m.Tlo,
+        ),  # (0, None),
+        initialize=lambda m, sec, tray, comp: m.T0[sec, tray] / m.Tlo,


I like this way of defining bounds and initializations!

Addressed while handling the newer reduced-temperature issue in a70395b: m.Tr now initializes from TC / T0, with a regression test for initial values staying within bounds.

Not applicable as a change request; this was informational. The reduced-temperature bounds and initializer remain, and tests/test_pr47_regressions.py now covers both bounds and algebra equivalence.

bernalde · 2024-08-14T01:35:43Z

-                        m.prop[comp, 'TC']
-                        / m.T[1, n_tray]
+                        m.Tr[1, n_tray, comp]
+                        # m.prop[comp, 'TC']


Why leave these lines here? If the code works, remove the comments as they become confusing

Addressed in a70395b by removing the stale commented TC / T algebra from the vapor-composition expressions.

Addressed in a70395b and preserved in 90e289d. The stale commented TC / T algebra was removed.

bernalde · 2024-08-14T01:36:06Z

                )
            )
-            / m.P[1, n_tray]
+            # / m.P[1, n_tray]


Remove comment as pressure multiplied in the other side of the equality

Addressed in a70395b. The stale commented pressure-division lines were removed from the vapor-composition expressions.

bernalde · 2024-08-14T01:36:59Z

+        """
+        return m.Tr[4, n_tray, comp] * m.T[4, n_tray] == m.prop[comp, 'TC']
+
    @disj.Constraint(m.comp, doc="Top scetion 4 vapor composition")


Typo here in section

Addressed in a70395b by fixing scetion to section in both top-section constraint docs.

Addressed in a70395b. The top-section scetion typo was corrected to section.

bernalde · 2026-05-08T12:52:40Z

This should be resolved by Pyomo/pyomo#3880

bernalde

Blocking issues:

gdplib.hda.HDA_model() no longer constructs because the new m.gamma scalar Param is overwritten by the existing indexed activity-coefficient m.gamma Var, and compressor/valve expressions then try to use that IndexedVar as a scalar heat-capacity ratio.
The new Kaibel m.Tr variable is initialized inconsistently with its definition (TC / T), producing 381 out-of-bounds Tr initial values during build_model() in the Pixi environment.

Nonblocking issues:

git diff --check reports trailing whitespace on the added Kaibel lines, and the changed-file black check says gdplib/kaibel/kaibel_solve_gdp.py would be reformatted.

Questions:

None.

Tests run and outcomes:

Discovered commands from the PR branch README/CI and current repo conventions. The PR worktree has no tests/, pyproject.toml, or Pixi manifest, so I used /home/bernalde/repos/gdplib/pixi.toml with cwd /tmp/gdplib-pr47 to run PR code on Python 3.12.13 / Pyomo 6.10.0.
/home/bernalde/.pixi/bin/pixi run --manifest-path /home/bernalde/repos/gdplib/pixi.toml --frozen python -m pytest tests/ -v --tb=short: failed because tests/ does not exist in the PR branch.
Kaibel build/initializer check: failed the new Tr consistency check; 381 Tr values are outside bounds.
Kaibel TransformationFactory('gdp.hull').apply_to(m): passed (hull ok).
HDA HDA_model() construction: failed with TypeError: unsupported operand type(s) for -: 'IndexedVar' and 'float' at m.gamma - 1.0.
black -S -C --target-version py310 --check --diff gdplib/kaibel/kaibel_solve_gdp.py gdplib/hda/HDA_GDP_gdpopt.py: failed; Kaibel would be reformatted.
flake8 gdplib/ --count --select=E9,F63,F7,F82 --show-source --statistics: passed (0).
typos --config ./.github/workflows/typos.toml: failed on existing repository typos, including old Kaibel scetion strings.
git diff --check origin/kaibel...HEAD: failed on trailing whitespace in added Kaibel lines.

The PR should not be merged as-is. I would not merge this until the blocking issues above are addressed.

bernalde · 2026-05-10T16:52:31Z

    m.alpha = Param(initialize=0.3665, doc="compressor coefficient")
    m.compeff = Param(initialize=0.750, doc="compressor efficiency")
-    m.gam = Param(initialize=1.300, doc="ratio of cp to cv")
+    m.gamma = Param(initialize=1.300, doc="ratio of cp to cv")


Blocking: This rename collides with the existing m.gamma = Var(m.abs, m.compon, ...) later in HDA_model(). Pyomo replaces this scalar Param with the IndexedVar, and HDA_model() now fails during construction in build_compressor() with TypeError: unsupported operand type(s) for -: 'IndexedVar' and 'float' when evaluating m.gamma - 1.0. This HDA change is unrelated to the Kaibel hull fix. Please restore m.gam for the heat-capacity ratio, or choose a non-conflicting name and update all scalar references without overwriting the activity-coefficient m.gamma variable.

Addressed in a70395b by renaming the HDA scalar to m.heat_capacity_ratio and updating the dependent equations. tests/test_pr47_regressions.py now checks that m.gamma remains indexed.

Addressed in 90e289d. The heat-capacity ratio is back to m.gam; the existing indexed activity-coefficient m.gamma is no longer overwritten, and the regression covers both.

bernalde · 2026-05-10T16:52:31Z

+            m.prop[comp, 'TC'] / m.Tup,
+            m.prop[comp, 'TC'] / m.Tlo,
+        ),  # (0, None),
+        initialize=lambda m, sec, tray, comp: m.T0[sec, tray] / m.Tlo,


Blocking: m.Tr represents TC / T because the new reduced-temperature constraints enforce m.Tr[...] * m.T[...] == m.prop[comp, 'TC'], but this initializer uses T0 / Tlo. In the Pixi environment this leaves 381 Tr entries outside their own bounds and emits W1002 warnings during build_model(), giving solvers a bad initial point for the newly added reformulation variable. Initialize with m.prop[comp, 'TC'] / m.T0[sec, tray] or another value consistent with the bounds, and add a regression check that builds Kaibel and applies TransformationFactory('gdp.hull').

Addressed in a70395b by changing the Kaibel m.Tr initializer to TC / T0. The new regression tests cover Tr bounds and both gdp.hull and gdp.bigm transformation construction.

Addressed in a70395b and extended in 90e289d. m.Tr initializes as TC / T0, transform tests cover Big-M and Hull, and the new regression checks algebra equivalence to the original TC / T vapor expression.

bernalde · 2026-05-10T19:23:12Z

Pushed commit:

a70395b Address PR 47 review feedback

Main changes:

Renamed the HDA scalar heat-capacity ratio to m.heat_capacity_ratio so it no longer overwrites the indexed m.gamma variable.
Fixed the Kaibel reduced-temperature initializer to use TC / T0, restored the Ruczika-D method comments, removed stale commented vapor-composition algebra, fixed the top-section typo, and made the added reduced-temperature constraint names/docstrings section-specific.
Added regression tests for HDA construction/gamma indexing, Kaibel Tr initial values staying within bounds, and Kaibel gdp.hull/gdp.bigm transformations.

Tests run:

/home/bernalde/.pixi/bin/pixi run --manifest-path /home/bernalde/repos/gdplib/pixi.toml --frozen python -m pytest tests/test_pr47_regressions.py -v --tb=short: 4 passed.
/home/bernalde/.pixi/bin/pixi run --manifest-path /home/bernalde/repos/gdplib/pixi.toml --frozen black -S -C --target-version py310 --check --diff gdplib/kaibel/kaibel_solve_gdp.py gdplib/hda/HDA_GDP_gdpopt.py tests/test_pr47_regressions.py: passed.
/home/bernalde/.pixi/bin/pixi run --manifest-path /home/bernalde/repos/gdplib/pixi.toml --frozen flake8 gdplib/ --count --select=E9,F63,F7,F82 --show-source --statistics: passed.
git diff --check and git diff --cached --check: passed.
/home/bernalde/.pixi/bin/pixi run --manifest-path /home/bernalde/repos/gdplib/pixi.toml --frozen typos --config ./.github/workflows/typos.toml: failed only on pre-existing unrelated indx/matche findings in gdplib/mod_hens/... and gdplib/gdp_col/initialize.py.

Comments intentionally not addressed:

The older request to verify optimal solution values for Big-M and Hull was not fully addressed with solver-backed solves; optional solver stacks are outside the default verification environment here. I added construction/transformation regressions for both transformations instead.
General discussion comments that did not request code changes were treated as context only.

Remaining risks/follow-up:

The Kaibel transformation tests do not prove optimal objective values.
The repository-wide typo check still has unrelated existing failures.

bernalde

Blocking issues:

HDA_model() no longer exposes the existing m.gam scalar component; the fix should avoid the m.gamma collision without renaming the pre-existing HDA component.
The Kaibel regression coverage still only proves that Big-M and Hull transform. The PR rewrites the vapor-composition algebra and adds m.Tr, so it still needs semantic equivalence or solve evidence for the changed model behavior.

Nonblocking issues:

The PR diff still fails git diff --check origin/kaibel...HEAD because several touched Kaibel files contain trailing whitespace or CRLF artifacts.
The restored Ruczika-D method comments still do not include a reference.

Questions:

None.

Tests run and outcomes:

Discovered commands from README.md and setup.py; this PR branch has no pyproject.toml, Pixi manifest, Makefile, package.json, or workflow files. I used the repository Pixi environment from /home/bernalde/repos/gdplib/pixi.toml and forced PYTHONPATH=/tmp/gdplib-pr47 because that shared .venv is editable-installed against the main checkout.
env PYTHONPATH=/tmp/gdplib-pr47 PYTHONDONTWRITEBYTECODE=1 /home/bernalde/.pixi/bin/pixi run --manifest-path /home/bernalde/repos/gdplib/pixi.toml --frozen pytest tests/ -v --tb=short -p no:cacheprovider: passed, 7 tests.
/home/bernalde/.pixi/bin/pixi run --manifest-path /home/bernalde/repos/gdplib/pixi.toml --frozen black -S -C --target-version py310 --check --diff .: passed.
/home/bernalde/.pixi/bin/pixi run --manifest-path /home/bernalde/repos/gdplib/pixi.toml --frozen flake8 gdplib/ --count --select=E9,F63,F7,F82 --show-source --statistics: passed.
/home/bernalde/.pixi/bin/pixi run --manifest-path /home/bernalde/repos/gdplib/pixi.toml --frozen typos --config ./.github/workflows/typos.toml: passed.
git diff --check origin/kaibel...HEAD: failed on trailing whitespace in touched Kaibel files.
GitHub lint/style-and-typos: passed.

The PR should not be merged as-is. I would not merge this until the blocking issues above are addressed.

bernalde · 2026-05-10T20:08:02Z

    m.alpha = Param(initialize=0.3665, doc="compressor coefficient")
    m.compeff = Param(initialize=0.750, doc="compressor efficiency")
-    m.gam = Param(initialize=1.300, doc="ratio of cp to cv")
+    m.heat_capacity_ratio = Param(initialize=1.300, doc="ratio of cp to cv")


Blocking: This removes the existing m.gam scalar component from HDA_model() and replaces it with m.heat_capacity_ratio. That avoids the m.gamma collision, but it also breaks existing HDA scripts that access the pre-existing heat-capacity ratio as model.gam, and this PR is otherwise about Kaibel. Please preserve the existing component name (m.gam) and use it in the compressor/valve expressions, while keeping the indexed activity-coefficient variable as m.gamma; update the regression to assert that model.gam still exists and model.gamma remains indexed.

Addressed in 90e289d. m.gam is restored in HDA_model(), compressor/valve references use it again, and the regression now asserts model.gam remains available while model.gamma remains indexed.

bernalde · 2026-05-10T20:08:02Z

-             |     |              |     |               |     |
-              -------> D           -------> C            -------> B
-   Figure 2. Sequence of columns for the separation of a quaternary mixture
+       Calculation of the theoretical minimum number of trays and initial


Nonblocking: The current PR diff still fails git diff --check origin/kaibel...HEAD; many touched Kaibel lines have trailing whitespace or CRLF artifacts. Please normalize the touched files to the repository line-ending style and remove trailing whitespace so the diff check is clean.

Addressed in 90e289d. Normalized touched Kaibel line endings and trailing whitespace; git diff --check origin/kaibel...HEAD now passes.

bernalde · 2026-05-10T20:08:02Z

        m.dHvap[comp] = dHvapb[comp] / m.Hscale

-    ## Heat capacity calculation for liquid and vapor phases using Ruczika-D method for each component in the feed, section, and tray
+    ## Heat capacity calculation using the Ruczika-D method for each component in the feed, section, and tray


Nonblocking: The Ruczika-D method comment is restored, but the requested reference is still missing. Please add a citation in this module or gdplib/kaibel/README.md so future maintainers can trace the property and enthalpy calculations.

Addressed in 90e289d. Added the Ruzicka-Domalski reference to gdplib/kaibel/README.md and corrected the local method comments to use that name.

bernalde · 2026-05-10T20:24:57Z

Pushed commit:

90e289d Address PR 47 review follow-ups

Main changes:

Restored the HDA heat-capacity ratio component as m.gam and updated the regression to preserve that public model component while keeping indexed m.gamma intact.
Added a Kaibel regression that compares the reduced-temperature vapor-composition reformulation against the original TC / T algebra for representative initialized tray/component data in all four sections.
Added the Ruzicka-Domalski heat-capacity reference in gdplib/kaibel/README.md and corrected the local method comments.
Normalized touched Kaibel line endings/trailing whitespace so the full PR diff passes git diff --check.
Added gam to the typo allowlist because it is an existing HDA model component name that must be preserved.

Tests run and results:

pytest tests/test_pr47_regressions.py -v --tb=short -p no:cacheprovider: 8 passed.
pytest tests/ -v --tb=short -p no:cacheprovider: 11 passed.
black -S -C --target-version py310 --check --diff .: passed.
flake8 gdplib/ --count --select=E9,F63,F7,F82 --show-source --statistics: passed.
typos --config ./.github/workflows/typos.toml: passed.
git diff --check origin/kaibel...HEAD: passed after commit.
GitHub lint/style-and-typos: passed.

Comments intentionally not addressed:

Solver-backed optimal objective evidence was not added. I added deterministic algebra-equivalence coverage instead; prior bounded SCIP benchmark runs for this PR reached time limits without a primal solution, so a default regression would be brittle.

Remaining risks or follow-up:

The new test verifies formulation equivalence for representative initialized algebra, not a full global solve certificate for Big-M or Hull.

bernalde

Blocking issues: none.

Nonblocking issues: none requiring inline comments. Residual risk: this review does not add solver-backed optimality evidence for the Kaibel objective; prior bounded SCIP benchmark attempts timed out without a primal solution, so I am treating the algebra-equivalence and GDP transformation regressions as adequate for this targeted Hull failure fix.

Questions: none.

Tests run and outcomes:

Targeted PR regressions: 11 passed.
Broader test suite: 11 passed.
Black check: passed.
Critical flake8 check: passed.
Typos check: passed.
Full advisory flake8 ran with exit-zero and reported existing repository-wide style issues.
GitHub lint/style-and-typos checks: passing.

This PR should be merged as-is.

tristantc and others added 2 commits August 13, 2024 12:13

Revised vapor composition constraints fix ZeroDivisionError when gdp.…

2838304

…hull reformulation is applied

replace gam with gamma

f798d9d

ZedongPeng requested a review from bernalde August 13, 2024 21:58

bernalde requested changes Aug 14, 2024

View reviewed changes

bernalde mentioned this pull request Aug 14, 2024

Update documentation of kaibel model #27

Merged

ZedongPeng force-pushed the tristantc/issue44 branch from f798d9d to 2838304 Compare August 19, 2024 20:44

bernalde requested changes May 10, 2026

View reviewed changes

Address PR 47 review feedback

a70395b

bernalde added 2 commits May 10, 2026 15:44

Fix repository lint checks

c19329c

Fix benchmark stdout restoration

8aceb54

bernalde requested changes May 10, 2026

View reviewed changes

Address PR 47 review follow-ups

90e289d

bernalde approved these changes May 10, 2026

View reviewed changes

bernalde merged commit e9a1cfb into kaibel May 10, 2026
2 checks passed

bernalde mentioned this pull request May 10, 2026

ZeroDivisionError in the Kaibel model #44

Closed

Conversation

tristantc commented Aug 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZedongPeng commented Aug 13, 2024

Uh oh!

bernalde left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bernalde commented May 8, 2026

Uh oh!

bernalde left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bernalde commented May 10, 2026

Uh oh!

bernalde left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bernalde commented May 10, 2026

Uh oh!

bernalde left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

tristantc commented Aug 13, 2024 •

edited

Loading