v3.1.0: perf + memory + ordering docs (#75, #113, #65) - #114
Merged
Conversation
…106 Documents the close-out plan for four open issues left after the v3.0.0 gams.transfer-engine arc: perf + memory (#65/#113), set element ordering (#75), domain-parent validation (#106) and docs nit, and the EPS remap (#39). Non-breaking items target v3.1.0; behavior breaks defer to v4.0.0. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Issue #75 (2020) reported to_gdx reordering Set elements on write. The exact example from the issue round-trips fine in isolation; the reorder appears when a later Set shares UELs with an earlier symbol. Both engines reproduce it identically (including gams_transfer<->gams_transfer, which never touches our gdxDataWriteStr loop), and gdxdump confirms the on-disk records are already in the reordered form -- so it's GAMS GDX file-format behavior, not a gdxpds bug. Records are stored sorted by global UEL-pool index, fixed by the first symbol to introduce each UEL. tests/test_set_ordering.py pins three cases over both engines: isolated order preserved; UEL collision reorders to pool order; writing the order-sensitive Set first restores order (the workaround). overview.md adds an "Element ordering and the UEL pool" subsection under "Set details" with the reproducer, the broader implication for any symbol indexed by reordered UELs, and the write-first workaround. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Extend test_engine_timing.py with a 500K-row 5-dim synthetic Parameter and a tracemalloc-based peak-Python-memory measurement around the to_gdx write call. Numbers feed a new "synthetic-write memory" table in pytest_terminal_summary so we can track the ratio (peak_MB / gdx_MB) across commits. Pre-optimization baseline on the synthetic 500K-row Parameter: engine gdx_MB peak_MB ratio seconds gams_transfer 6.25 88.64 14.18 0.22 gdxcc 6.78 53.43 7.89 5.46 Both engines run above the v3.1.0 target of <= 3x; the per-row Python overhead in the gdxcc write loop (~25x slower than gams_transfer on this workload) is the next thing to fix. Dim labels are constructed by treating the row index as a mixed-base integer over per-dim UEL pools, so every row is a unique tuple (gams.transfer rejects duplicate keys). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds two helper functions and tests that write the synthetic 500K-row Parameter via the raw SWIG gdxDataWriteStr loop and via raw gams.transfer, mirroring the gdxpds-engine measurement loop. Their seconds appear in the memory table as raw_gdxcc / raw_transfer, and a new "x raw" column shows each gdxpds engine's seconds / paired-raw ratio so the 1.3x (gdxcc) and 1.5x (gams_transfer) acceptance targets from the wrap-up plan can be read off every test run. Pre-optimization baseline on the synthetic workload: engine gdx_MB peak_MB ratio seconds x raw raw_transfer 6.25 42.85 6.85 0.18 - raw_gdxcc 6.78 68.66 10.13 2.38 - gams_transfer 6.25 88.64 14.18 0.22 1.26x (<= 1.5x target) gdxcc 6.78 53.43 7.89 5.07 2.13x (> 1.3x target) Takeaways for what to optimize: - gams.transfer time is already within target; only its peak memory (~2x the raw_transfer baseline) needs reducing -- driven by the per-symbol records.copy() in _transfer_engine._add_symbol. - gdxcc time needs to come down from 2.13x to <= 1.3x of raw -- the per-row Python overhead (str() per dim, isinstance per value, convert_np_to_gdx_svs's full DataFrame copy) is the next target. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
``pd.option_context("future.no_silent_downcasting", True)`` was wrapped
around the ``replace`` call to opt into pandas 3.x's "no silent
downcasting" behavior so a Parameter column carrying UNDEF (None)
wouldn't be collapsed back to NaN. On pandas 4.x that option is
deprecated and its emission triggers a ``Pandas4Warning`` every call --
~1,924 warnings across one ``pytest tests`` run.
pyproject already pins ``pandas >= 2.2``, where the "future" behavior is
the default for ``replace`` on object columns, so the option-context is
no longer load-bearing. Drop it; warnings go to zero with no test
behavior change (171 passed, both with and without the change).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the per-row Python overhead in GdxccEngine.write_symbol with a vectorized pre-pass: - Pre-stringify dim columns into per-column object ndarrays once (``astype(str)`` is a near-no-op view on already-string columns and amortizes the per-row ``str()`` into one allocation per numeric col). - Pre-materialize dim-lists in 100K-row chunks via ``np.column_stack(...).tolist()``, so the per-row write does a single list lookup instead of a Python list comp. Chunking bounds the peak sustained list-of-lists overhead independently of total row count (issue #65: a one-shot variant would hold ~2-3 GB on a 29M-row 7-dim Parameter). - New ``_coerce_value_col`` builds one float64 array per value column with all gdxpds-canonical special values pre-substituted to their GDX magic floats, including Python ``None`` -> GDX UNDEF (recorded BEFORE the float64 cast since None coerces to NaN and would otherwise be indistinguishable from genuine NA). Replaces the ``convert_np_to_gdx_svs`` full-DataFrame copy AND the per-row ``isinstance(v, Number)`` / ``v is None`` checks. The legacy 0.0-for-non-Number-non-None fallback is preserved for object columns with mixed types. - Special-case the Parameter inner loop (single value col) so the hot path doesn't iterate over a 1-element ``value_arrays`` list per row. Also: ``test_synthetic_write_memory`` now does separate time and memory passes. ``tracemalloc`` inflates wall time by an allocation-pattern- dependent factor, so the "seconds" column previously made gdxcc look ~2.13x raw when the actual wall-clock (without tracemalloc) was already ~1.18x. Two passes -- one outside tracemalloc for time, one inside for peak -- gives the table honest numbers. Pre/post on the 500K-row synthetic Parameter (no-tracemalloc time): engine seconds x raw memory_peak raw_gdxcc 0.36 - 69 MB gdxcc (pre) 0.77 2.13x 53 MB gdxcc (post) 0.43 1.11x 50 MB <-- 1.3x target met raw_transfer 0.17 - 43 MB gams_transfer 0.20 1.22x 89 MB <-- 1.5x target met gams_transfer time was already within target; only its peak memory (~2x raw_transfer) still needs work -- next commit. Adds ``dev/profile_gdxcc_write.py`` and ``dev/profile_raw_gdxcc.py`` as keepable cProfile probes used to localize the per-row overhead. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
TransferEngine._add_symbol was making a full ``symbol.dataframe.copy()`` per symbol before handing records to gams.transfer. On the 500K-row synthetic Parameter that copy alone was ~50 MB; it scales linearly with input size, with the dim columns (string UELs) dominating at large dimension counts. Build ``records`` instead as a DataFrame whose dim columns are views onto the user's ``symbol.dataframe`` (no data copy under pandas 2.2+ copy-on-write) and whose value column(s) are fresh ndarrays produced by a new per-column ``_substitute_value_col`` helper. The value-column allocations are unavoidable (we can't mutate the user's data, and we need to substitute eps/NaN/None to gt's SpecialValues encoding); the dim columns now stay shared. ``_substitute_value_col`` replaces the previous in-place ``_np_to_transfer_specials`` and uses the same vectorized SV pattern as the gdxcc engine: NaN -> NA, eps -> EPS, ``None`` (object-col only) recorded BEFORE the float64 cast and then overridden to UNDEF at the end so the UNDEF/NA distinction survives the float coercion. Result on the 500K-row synthetic Parameter: engine peak_MB (pre) peak_MB (post) vs raw_transfer raw_transfer 43 43 1.00x gams_transfer 89 70 1.62x (was 2.07x) Time also improved slightly (1.22x -> 1.06x raw) because we no longer copy the dim columns. All 171 tests pass. (Issue #65 -- the 18 GB report -- predates the gams.transfer engine and was actually the gdxcc path; the prior commit's removal of convert_np_to_gdx_svs's full-DataFrame copy is what reduces that case.) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Extends the synthetic-IO scaffold with read measurements (raw and gdxpds-mediated) and a 5M-row variant of every write+read benchmark gated by a new ``slow`` pytest marker. ``pytest tests`` runs the default-scale (500K-row) suite as before; ``pytest tests -m slow`` runs the scaling probe. Refactors test_engine_timing.py around two new helpers: - ``_build_synthetic_param(n_rows)`` -- programmatic Parameter builder used by both default and large fixtures; picks per-dim UEL pools so every row is a unique tuple even at 5M rows. - ``_record_io(engine, op, ...)`` -- the two-pass time/memory measurement wrapper, with one ``time_fn`` outside tracemalloc (honest wall-clock) and one ``mem_fn`` inside it (peak Python memory). Replaces the duplicated time + ``_peak_python_memory`` blocks scattered across the prior tests. New raw read floors via ``_raw_gdxcc_read`` (bare ``gdxDataReadStr`` loop into Python lists) and ``_raw_transfer_read`` (``Container.read``). The conftest's ``pytest_terminal_summary`` now groups the synthetic-IO rows by ``(rows, op)`` so default-scale and slow-scale tables print in separate sections labelled by row count. Fix incidental: the engine-memory section was inside the ``if engine_timings: ...`` block, so slow-only runs (which don't populate engine_timings) never rendered the table. 5M-row synthetic Parameter (slow tests): op engine seconds x raw peak_MB ratio read raw_transfer 0.26 - 206 3.0x read raw_gdxcc 3.69 - 1,409 20.8x read gams_transfer 0.71 2.67x 525 7.8x read gdxcc 6.31 1.71x 2,420 35.7x write raw_transfer 1.80 - 305 4.9x write raw_gdxcc 3.93 - 687 10.1x write gams_transfer 1.93 1.07x 566 9.1x (<= 1.5x target) write gdxcc 4.24 1.08x 358 5.3x (<= 1.3x target) Write ratios at 5M are essentially identical to those at 500K (1.03x -> 1.08x for gdxcc, 1.18x -> 1.07x for gams_transfer), confirming the optimizations scale linearly to issue #65's order of magnitude. Read perf is documented but not optimized in this commit (out of the v3.1.0 wrap-up scope). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two engine-side read refactors that drop the per-row Python overhead and per-symbol DataFrame copies that dominated the read-time and read-memory profiles on the 500K-row and 5M-row synthetic Parameter. gdxcc engine (``_load_one``): - Replace the row-list materialization ``data = [elements + [...] for ...]`` followed by ``convert_gdx_to_np_svs`` (a full-DataFrame copy + per-cell ``.replace`` work) with pre-allocated per-column ndarrays: ``num_dims`` object arrays for dims, one float64 (or object for Set element-text) per value column. The per-row gdxcc call now stores directly into the column arrays. - New ``_gdx_to_np_svs`` helper does per-column vectorized substitution of gdxcc magic floats to numpy SVs (UNDEF -> None forces object dtype, NA -> NaN, +/-Inf -> +/-inf, EPS -> machine eps). - Empty-symbol short-circuit: build an empty object-dtype DataFrame for zero-record Parameter/Variable/Equation, matching the transfer engine's empty-records branch. gams_transfer engine (``_translate``): - Drop the full-DataFrame ``out = values.copy()`` from ``_convert_transfer_specials`` and replace with per-column ``_convert_transfer_value_col`` that returns one fresh ndarray per call. - Decategorize dim columns via ``cat.categories[cat.codes]`` instead of ``.astype(str)``. gt stores dim columns as ordered categoricals with int8/int16 codes; ``categories[codes]`` returns an object ndarray of shared string references (only ``len(categories)`` distinct Python str objects exist regardless of row count) without an intermediate type-conversion DataFrame. - Build the output DataFrame from a positional dict + rename so the ``pd.concat([dim_data, value_data], axis=1)`` allocation is gone. Pre/post on 500K-row synthetic Parameter: op engine peak_MB (pre -> post) seconds (pre -> post) read gdxcc 241 -> 150 0.62 -> 0.47 read gams_transfer 53 -> 53 0.07 -> 0.08 5M-row (slow tests): op engine peak_MB (pre -> post) seconds (pre -> post) read gdxcc 2,420 -> 1,501 6.31 -> 3.99 read gams_transfer 525 -> 525 0.71 -> 0.91 The transfer-read peak (~52 MB at 500K, ~525 MB at 5M) is now near-optimal for the user-facing API contract: roughly half is gt's internal categorical-encoded storage during ``Container.read()`` and half is the materialized gdxpds DataFrame the user sees -- both must coexist briefly during translation. Also documents the synthetic-IO benchmarks and the ``-m slow`` opt-in in [dev/README.md](dev/README.md). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
``GdxSymbol.dataframe.setter`` did ``df = data.copy()`` on every assignment -- a full deep copy of the caller's DataFrame. The setter's subsequent work only mutates column metadata (``df.columns = ...``, ``self.dims = dim_cols``) and optionally appends default value columns or fixes up a Set's element-text values; in pandas 2.2+ copy-on-write mode, a shallow copy is enough to keep those local to ``df`` without touching the user's frame. On the 500K-row synthetic Parameter the deep copy was ~22 MB of allocation (mostly the BlockManager + dim-column pointer arrays -- the underlying strings were already shared with the source via intern/pool), scaling linearly with input rows. At the 5M-row scale this single setter call alone allocated ~220 MB per symbol. Because the setter sits on every write path -- through ``to_gdx`` for both engines and through the engine-side read path for ``symbol.dataframe = df`` -- removing the deep copy benefits everything downstream, not just one engine. Pre/post on the 500K-row synthetic Parameter (write peaks): engine peak_MB (pre -> post) x raw_*_write gdxcc 50 -> 27 0.73x -> 0.40x gams_transfer 70 -> 47 1.62x -> 1.09x 5M-row (slow tests): engine peak_MB (pre -> post) x raw_*_write gdxcc 358 -> 129 0.52x -> 0.19x gams_transfer 566 -> 338 1.86x -> 1.11x At 5M rows, ``gdxcc`` write now peaks ~5x below the raw_gdxcc baseline (the raw baseline pre-builds all dim_lists in one shot; gdxpds chunks). ``gams_transfer`` write is essentially at parity with ``raw_transfer`` (1.11x is gt's internal categorical conversion, which both paths share). All 175 tests pass; no inadvertent mutation of the caller's frame (the subsequent setter operations are CoW-safe under pandas 2.2+). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The prior framing (in both the user-facing overview docs and the ``to_gdx`` / ``get_subset_relationships`` docstrings) read as if only Sets carry domain information, when in fact every symbol type -- Parameter, Variable, Equation, Set -- can declare a strict domain over a parent Set. The semantics differ: - **Set on Set** is a *subset* relationship: ``set sub_a(a)`` declares ``sub_a`` is a subset of ``a``. - **Parameter / Variable / Equation on Set** is an *indexed-over* relationship: ``parameter p(a)`` declares that ``p`` is defined on ``a``'s elements. The mechanics in GDX (``gdxSymbolSetDomain``) and in gdxpds (``GdxSymbol.domain``, ``GdxSymbol.domain_type``) are identical for both cases. The parent in each domain slot must be a Set or Alias-of-Set -- ``gdxSymbolSetDomain`` enforces this at write time. Changes: - [doc/source/overview.md] rename ``#### Subset (Domain) Relationships`` to ``#### Domain Relationships`` (under "Set details" stays, since that's the parent section; the heading text changes); rewrite the lead paragraph to spell out both relationship kinds; add a Parameter-on-Set example next to the existing Set-on-Set "Setting on write" block (verified to round-trip on both engines and read back with ``domain_type == REGULAR``); update internal cross-references to the renamed anchor (``#subset-domain-relationships`` -> ``#domain-relationships``) and the surrounding inline mentions. - [src/gdxpds/write_gdx.py] soften the ``to_gdx`` ``domains=`` docstring from "Optional subset/domain relationships" to "Optional domain relationships", and add a sentence noting that any symbol type can carry a domain and the parent must be a Set or Alias. - [src/gdxpds/read_gdx.py] same softening on ``get_subset_relationships``'s docstring. Sphinx docs build cleanly with ``-W --keep-going``; all 175 tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR prepares v3.1.0 with performance/memory reductions in both GDX engines, documents GDX UEL ordering behavior, and updates domain relationship documentation.
Changes:
- Vectorizes/special-cases gdxcc and gams.transfer read/write paths to reduce per-symbol copies and per-row overhead.
- Adds synthetic timing/memory benchmark scaffolding, slow-test gating, and profiling scripts.
- Documents UEL-pool ordering semantics and broadens domain docs from subset-only wording to all domain-bearing symbol types.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/gdxpds/_gdxcc_engine.py |
Reworks gdxcc read/write paths with preallocated arrays, chunked dim materialization, and vectorized special-value conversion. |
src/gdxpds/_transfer_engine.py |
Reduces transfer-engine DataFrame copies and substitutes value columns independently. |
src/gdxpds/gdx.py |
Changes GdxSymbol.dataframe setter to shallow-copy incoming DataFrames. |
src/gdxpds/special.py |
Removes pandas future option wrapper around GDX-to-numpy special-value replacement. |
src/gdxpds/write_gdx.py |
Updates to_gdx domains= documentation. |
src/gdxpds/read_gdx.py |
Updates get_subset_relationships domain documentation. |
src/gdxpds/__init__.py |
Bumps package version to 3.1.0. |
tests/test_engine_timing.py |
Adds synthetic large-row timing/memory measurements and raw-engine baselines. |
tests/conftest.py |
Adds terminal-summary reporting for synthetic memory/timing rows. |
tests/test_set_ordering.py |
Adds regression coverage for Set ordering under UEL collisions and workaround ordering. |
pyproject.toml |
Registers slow marker and excludes slow tests by default. |
doc/source/overview.md |
Adds UEL ordering docs and expands/renames domain relationship documentation. |
dev/README.md |
Documents benchmark and profiling workflows. |
dev/profile_gdxcc_write.py |
Adds a cProfile helper for mediated gdxcc writes. |
dev/profile_raw_gdxcc.py |
Adds a cProfile helper for raw gdxcc baseline writes. |
dev/modernization-wrapup.md |
Adds release planning and follow-up context. |
CHANGES.txt |
Adds v3.1.0 release notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
268
to
275
| *indexed-over* relationship -- and the parent named in each slot must itself be a | ||
| Set or Alias-of-Set. Each entry maps a child symbol's name to a list or tuple of | ||
| its parent Set names, one per dimension, with ``None`` slots mapping to the GAMS | ||
| wildcard (``'*'``). When provided, the resulting :class:`Translator` (1) topologically | ||
| sorts ``dataframes`` so each parent precedes its children and (2) wires up strict | ||
| :c:func:`gdxSymbolSetDomain` writes for each listed child. Any invalid input | ||
| (unknown parent name, wrong type, wrong length, cyclic references) raises | ||
| :class:`DomainError`. |
Previously, passing a Parameter/Variable/Equation as the parent in a
strict domain -- via either ``GdxSymbol.domain = [...]`` or
``to_gdx(..., domains={...})`` -- silently fell back to a relaxed write:
``gdxSymbolSetDomain`` rejects the call at write time, gdxpds logged an
info-level fallback, and the resulting GDX recorded the wrong (relaxed)
domain type while the caller's code thought they got strict.
Add the same Set-or-Alias-of-Set check the ``alias_of`` setter already
does, in two places:
- ``GdxSymbol.domain.setter`` ([src/gdxpds/gdx.py]): per-entry check on
the resolved GdxSymbol references, raising ``DomainError`` with the
parent's actual data_type before any write begins.
- ``Translator.__wire_domains`` ([src/gdxpds/write_gdx.py]): same check
on each resolved parent before assigning, with a ``to_gdx: domains[...]``
error prefix so users get a callsite-relevant message.
Open Question #1 from dev/modernization-wrapup.md is resolved on the
way: both the gdxcc and gams.transfer engines accept an Alias as a
domain parent, so the rule admits Alias-of-Set (matching the
``alias_of`` setter's existing pattern). Probed empirically against a
local GAMS install by writing a Parameter with ``domain=[at]`` for an
Alias ``at`` of Set ``t``; both engines round-trip with
``domain_type == REGULAR`` and the recorded parent resolving back to
the Alias.
New regression tests in tests/test_domain.py:
- ``test_domain_setter_rejects_non_set_parent``
- ``test_to_gdx_domains_rejects_non_set_parent``
- ``test_domain_accepts_alias_parent``
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Drives the v3.1.0 work from dev/modernization-wrapup.md:
GdxSymbol.dataframesetter is now shallow;convert_np_to_gdx_svs/_np_to_transfer_specials/TransferEngine.write_symbolno longer take full-frame copies), chunked dim-list materialization on gdxcc writes.overview.mdand pinned bytests/test_set_ordering.py. Issue already closed.overview.md; docs now make explicit that any symbol type can carry a strict domain over a Set or Alias-of-Set parent. Added a Parameter-on-Set example.slowpytest marker gates a 5M-row scaling probe (pytest -m slow); the default suite runs a 500K-row variant. Synthetic-IO scaffold records raw-engine baselines so per-commit ratios are visible. Dropped a deprecatedpd.option_contextwrapper, silencing ~1,924Pandas4Warningemissions per test run.Test plan
pytest tests— 175 passed, 8 deselected (slow)pytest tests -m slow— 8 passed at 5M rowsruff check ./ruff format --check ./pyrightclean on edited filescd doc; .\make.bat html SPHINXOPTS=\"-W --keep-going\"builds cleanlyoverview.mdround-trips on both enginespytest testson each pinned-GAMS venv (per dev/README.md) before publishing the release tagIssues closed by merging
🤖 Generated with Claude Code