Repository navigation
Consolidate static-feature support into timegan.py; fix tests, CI, lint findings - #3
Merged
Merged
Conversation
…n_timed Both were hardcoded (gamma=1, both loss weights=100, matching the original TimeGAN paper) with no way to override via the parameters dict, even though every other hyperparameter already flows through it. G_loss_S (supervised: MSE between a real sequence's next embedded step and the generator's teacher-forced prediction of it) and G_loss_V (moment matching: batch-aggregate mean/std alignment) are both regression-style losses, not adversarial ones -- for any timestep where multiple real sequences share a similar state but diverge afterward, their MSE-optimal target is the conditional average of those futures, not a faithful sample of one. At the paper's default weight (100x) this can dominate the adversarial term (G_loss_U) in the generator's total loss, biasing it toward "the average-looking sequence" regardless of its noise input -- a plausible explanation for mode collapse observed training against Traces_GAN's pulse data despite reasonable-looking phase 3 loss curves. Uses parameters.setdefault(...) rather than a plain local default (same pattern already used for parameters["dim"] = dim just above) so the resolved value -- whether caller-supplied or default -- lands back in the caller's own parameters dict. Callers that log/persist that dict (e.g. into a run's metadata, as Traces_GAN's alderaan_debug_run scripts do) get these recorded automatically, with no additional plumbing. Scoped to train_timegan_timed only, the function every current pipeline script actually calls. train_timegan and load_timegan have their own separate (unused by train_timegan_timed) hardcoded gamma=1 and were left alone -- train_timegan is still exercised by this repo's own test.py/ tutorial.py, and load_timegan only rebuilds the graph for checkpoint restore/inference, where gamma's value is never evaluated. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PF8L7MkAiLARxWd2dSGSmq
timegan_static.py's static-feature path had never actually run
successfully, in this repo's history:
- Broken package import: `from utils import ...` collided with an
unrelated PyPI package named `utils` instead of resolving to
timegan.utils.
- unpack_parameters() was never updated for the extra static_dim/
static_z_dim params embedder/recovery/generator/discriminator all
try to unpack -- immediate ValueError.
- recovery()'s signature never gained the `S` param its own body
referenced -- NameError.
- Malformed tf.concat() calls (missing axis argument, and mixing a
per-timestep tensor with a per-event one with no broadcast/tiling
scheme), a typo'd biderectional_dynamic_rnn (no such method), and a
bare undefined `sigmoid` activation function.
- train_timegan/load_timegan never accepted static feature data as an
argument at all, and called the (now-fixed) component functions
with the wrong number of arguments.
Fixed all of the above. Static features are now genuinely embedded/
discriminated (HS/ES/XS_tilde/XS_hat/YS_* are real graph nodes), but
intentionally don't participate in any loss term yet -- that's a
separate design decision (how should static reconstruction/
adversarial terms combine with the temporal ones?) left for later.
The temporal path's own computation is unchanged from before this fix
in every case, so this can't regress non-static behavior.
Also fixes a smaller pre-existing bug: data_loading.py's
real_data_loading() used a bare relative path for the energy dataset
("data/energy_data.csv") that only worked by accident depending on
caller cwd, unlike the stock branch's proper package-resource loading.
Adds tests/test_training.py: real short training runs against the
bundled stock/energy datasets plus synthetic data for the static path,
using statistical/structural assertions (shape, finite, sigmoid-
bounded range) rather than exact golden values -- verified directly
that this TF1 graph-mode code is NOT bit-reproducible in this
environment even with every seed fixed and execution forced single-
threaded, so an exact-match test would be flaky by construction.
Adds report_training_comparison.py: a manual capture/compare tool for
the planned embedder/recovery/generator/discriminator consolidation
between timegan.py and timegan_static.py. Captures N repeated runs
(same code, different seeds) to characterize normal run-to-run noise
on each side of a change, then reports whether the "after" side looks
like more of the same noise or a real shift -- a single before/after
run would be indistinguishable from noise given the non-determinism
above.
Removes tests/test_foo.py and timegan/foo.py, unused cookiecutter
scaffold left over from this project's template.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DbnvNbSohNTTvL8MDx5RUU
train_timegan was a full standalone copy of train_timegan_timed's graph- building/training-loop code, predating the phase/resume/timing additions there -- the same "same fix needs applying in two places" risk that let timegan_static.py's copy of this same code drift until it no longer even ran (fixed in the previous commit). Confirmed train_timegan_timed's defaults (gamma=1, g_loss_s_weight=g_loss_v_weight=100, num_generate=None -> one generated sequence per training event) already reproduce train_timegan's previously-hardcoded behavior exactly, so this changes nothing for any existing caller not passing those parameter keys. train_timegan now just calls train_timegan_timed with seconds=inf (so the time-budget check can never trigger an early return), phase=1, new=True, and unwraps the (phase, generated_data) result -- raising instead of silently returning partial output in the two cases that would indicate something unexpected (phase==-1 error, or, if the seconds=inf assumption is ever wrong, any phase != 4). Two side effects, both additive: also writes <filename>_history.jsonl now (harmless extra output), and names the Saver checkpoint pointer file after `filename` instead of TF's shared default bare "checkpoint" (an improvement -- that shared pointer file is exactly what used to get clobbered across different runs/filenames). Verified via tests/test_training.py's test_train_timegan_base_smoke and report_training_comparison.py (capture --fn base): output shape/dtype and the "num_generate defaults to matching training-event count" behavior are unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DbnvNbSohNTTvL8MDx5RUU
Merges embedder/recovery/generator/discriminator's static-feature capability (fixed in the previous commits) directly into timegan.py's own versions of those functions, via a new optional S=None parameter: when S is None, behavior is byte-for-byte unchanged from before this had any static-feature support at all (same graph ops built, nothing extra even constructed); when S is given, the function also embeds/ discriminates it and returns a (temporal, static) tuple instead of a single value. Existing callers passing no S are completely unaffected. Extends train_timegan_timed, train_timegan, and load_timegan the same way via an optional ori_data_static=None parameter -- when given, builds S/S_z placeholders, threads them through batching (via batch_generator's new optional `static` param, itself backward compatible: omitting it keeps the original 2-tuple return) and every feed_dict, and wires up the branch-specific graph construction. Static features are genuinely embedded/discriminated (HS/ES/XS_tilde/XS_hat/ YS_* are real graph nodes), but still intentionally don't participate in any loss term -- that's the same open design decision noted in the previous commits, not resolved here. With that, timegan_static.py has nothing left that isn't now also in timegan.py, so it's deleted outright -- not kept as a compatibility shim, since (a) it never successfully ran in this repo's history until the previous two commits fixed it, so there's no working behavior for a shim to preserve, and (b) a GitHub code search turned up zero public repos importing `timegan.timegan_static` specifically. Updates tests/test_training.py and report_training_comparison.py to call the merged functions with ori_data_static= instead of importing from the now-deleted module. All 5 tests still pass; report tool verified against all three training entry points (timed/base/static). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DbnvNbSohNTTvL8MDx5RUU
Real bugs, would fail if actually exercised: - main_timegan.py: called `timegan(ori_data, parameters)`, treating the imported module as a callable instead of `timegan.train_timegan(...)` -- TypeError on the first real line of main(). - pyproject.toml: removed the `[project.scripts] test = "tests.tutorial: start"` entry point -- tests/tutorial.py has no `start` function (no functions at all, just top-level script code), so this pip-installed `test` command was never callable. Real tests already run via `make test`/pytest; this was a misleadingly-named, non-functional duplicate, not worth resurrecting. - test.sh: `python3 tutorial.py` -- wrong relative path, the file is at tests/tutorial.py. Fixed the path instead of removing the script. - Dockerfile: pinned python:3.9-slim-buster, contradicting pyproject. toml's `requires-python = ">=3.10,<3.11"` and CONTRIBUTING.md's explicit "3.10 only" -- likely fails at `poetry install` or produces an unsupported environment. Also its CMD pointed at timegan/foo.py, deleted by an earlier commit on this same branch -- pointed it at main_timegan.py (the package's actual demo entry point) instead. Stale documentation: - docs/index.md carried badges/links for zkromerUCD/TimeGAN, a different (predecessor?) repo -- README.md at the repo root already had this right (det-lab/TimeGAN-Static); docs/index.md just never got updated to match. - env.def's help text said "version 0.1.8"; pyproject.toml is at 0.1.13. - embedder()'s docstring pointed at "timegan_static.py's git history" for context on why static features aren't concatenated into the temporal branch -- that file no longer exists after the previous commit. Reworded to explain the reasoning inline instead of pointing at a deleted file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DbnvNbSohNTTvL8MDx5RUU
training entry points load_timegan duplicated all of train_timegan_timed's graph-building code (placeholders through the loss/optimizer ops) -- the last remaining big duplication after this branch's earlier consolidation work. It needs those ops built even though it never trains, purely so its Saver's variable list matches what was checkpointed (AdamOptimizer creates slot variables that get saved alongside the trainable ones). That duplication had already drifted: load_timegan's copy hardcoded gamma=1 and the paper's fixed 100x loss weights, while train_timegan_ timed's copy had since gained the configurable gamma/g_loss_s_weight/ g_loss_v_weight. Harmless in practice (load_timegan never executes those loss ops -- only X_hat, for generation), but a real, easy-to- miss inconsistency sitting right next to the configurable version, and exactly the kind of drift this whole branch has been about removing. Extracted the shared logic into _build_timegan_graph(parameters, max_seq_len, dim, static_dim), returning a dict of every tensor/op either caller needs. train_timegan_timed and load_timegan both call it and unpack just what they use; the two-branch (static features or not) reasoning that used to be duplicated in both call sites now lives in exactly one place. Verified via the full test suite (all 5 tests, including the static- feature load/restore round trip -- the riskiest path, since it depends on the checkpoint's graph shape matching exactly) and report_training_comparison.py against both entry points. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DbnvNbSohNTTvL8MDx5RUU
…ackage unpack_parameters returned iterations, batch_size, and a hardcoded gamma=1 that every one of its 5 callers (embedder/recovery/generator/ supervisor/discriminator) unpacked into a local variable and never used again -- dead bindings in every single call site. The gamma was worse than dead: it was a live lie for anyone who trusted it, since train_timegan_timed's own real, configurable gamma (read straight from parameters["gamma"] in _build_timegan_graph) is what's actually used during training and shadows this one entirely. Trimmed the return to just what's used: hidden_dim, num_layer, module, dim. Moved timegan/test.py to tests/full_demo.py: despite the name, it was never a pytest test (no test functions, just top-level script code that trains a real model and computes discriminative/predictive metrics) -- and it lived inside timegan/, the actual shipped package directory, so it would have been bundled into the installed wheel. It's also a near-duplicate of tests/tutorial.py (same demo, just at production iteration counts with metrics/visualization added) -- the same "two very similar functions" pattern this branch has been removing elsewhere, just never applied here. Named to not match pytest's test_*.py/*_test.py discovery patterns (same reason tutorial.py was already safe), so it won't get accidentally collected and executed as a real test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DbnvNbSohNTTvL8MDx5RUU
pytest --doctest-modules tests (what tox.ini's test command, and so CI's tox job, actually runs) imports every .py under tests/ to scan for doctests. Both files ran real TimeGAN training unconditionally at module level, so importing them for doctest-scanning silently kicked off a real (in full_demo.py's case, 10,000-iteration) training run. Confirmed the hang directly with a timeout-wrapped collect-only run before fixing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P5kHya3EZkoBQarkiwdJmL
- tests/test_training.py: hardcoded /tmp/test_* checkpoint paths -> pytest's tmp_path fixture. - report_training_comparison.py: hardcoded /tmp/report_cmp_* paths -> tempfile.gettempdir(); noqa the git-rev-parse subprocess call (fixed argv list, no shell involved -- not the injectable pattern S603/S607 are meant to catch). - timegan/timegan.py: noqa the two long-message RuntimeErrors in train_timegan rather than introducing dedicated exception classes for two internal error paths. Also includes ruff-format's own reformatting of these files (one-arg- per-line call style) from running the formatter as part of this pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P5kHya3EZkoBQarkiwdJmL
Confirmed via deptry (DEP002) and grep: nothing in the codebase does `import utils`/`from utils import ...` (everywhere correctly uses timegan.utils). This spurious PyPI package -- added by mistake during the poetry [tool.poetry] -> PEP 621 [project] migration -- is what was shadowing timegan_static.py's intended local import before that file was fixed and later folded into timegan.py. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P5kHya3EZkoBQarkiwdJmL
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
_build_timegan_graphso train_timegan_timed and load_timegan share one graph-construction implementation instead of two copies that had already drifted (mismatched gamma/loss-weight defaults).pytest --doctest-modules tests(what tox.ini's test command runs) hung importing tests/tutorial.py and tests/full_demo.py, since both ran real training unconditionally at module level with no__main__guard.timegan(...)vstimegan.train_timegan(...)call in main_timegan.py, and a dead[project.scripts]entry point.Verification
Ran the full CI-equivalent suite locally against the exact toolchain versions pinned in poetry.lock:
pre-commit run --all-files: all hooks passruff check/ruff format --check: cleanmypy: clean (see caveat below)deptry: cleanpytest --doctest-modules tests --cov: 5/5 pass, 93% coverage on timegan.pypoetry check --lock: cleanmkdocs build -s: cleanKnown gap, not addressed in this PR: every file under
timegan/has# mypy: ignore-errorsat the top, so the strict[tool.mypy]config in pyproject.toml is currently inert everywhere. Flagging for a separate decision on scope (leave as-is / enable for new code / full annotation pass).Test plan
pytest --doctest-modules tests --covpasses (5 tests)pre-commit run --all-filespassesmkdocs build -spassespoetry check --lockpasses🤖 Generated with Claude Code
https://claude.ai/code/session_01P5kHya3EZkoBQarkiwdJmL