Skip to content

Fix code review findings in the Python and R packages - #1

Merged
melhzy merged 7 commits into
mainfrom
fix/code-review-findings
Oct 2, 2026
Merged

melhzy merged 7 commits into
mainfrom
fix/code-review-findings

Conversation

@melhzy

@melhzy melhzy commented Oct 2, 2026

Copy link
Copy Markdown
Owner

Fixes from a code review of the Python and R packages, one commit per step.

Changes

  1. R seeds now match Python. R took the first 7 hex digits of the MD5 digest, so the same string gave a different seed in each language, contrary to the paper. seed_number is now the MD5 digest of the UTF-8 input modulo 2^32, as in Python. In the same commit:
    • SeedHashGenerator$new() defaults to 0 to 2^31−1. It used −1e9 to 1e9, which disagreed with create_seedhash(), Python and the tests; with that default, create_seedhash() always failed in generate_seeds().
    • generate_seeds() returns integers, no longer overflows to NA near 2^31−1, and works on the full integer range.
    • generate_seeds() pins the RNG kind and restores the caller's .Random.seed.
    • Fields are read-only.
    • New set_seed(), because set.seed(seed_number) fails when the seed is above 2^31−1.
  2. R tests can now fail CI. The old script printed FAILED but always exited 0, and two of its tests were failing. The tests are now a testthat suite, which CI installs and runs.
  3. Python: infinite loop on seed collisions. A repeated seed value could make itself its own ancestor, so SeedExperimentManager.add_experiment_result() never returned.
  4. Python packaging.
    • Requires Python 3.8, since typing.Literal broke the import on 3.7.
    • The extras moved to pyproject.toml. setuptools ignored the ones in setup.py, so pip install seedhash[torch] installed nothing extra.
    • Requires torch 1.11, which warn_only=True needs.
  5. Python correctness.
    • Binary classification metrics assumed labels 0 and 1.
    • Multi-class F1 now averages per-class F1.
    • (n,) against (n, 1) inputs no longer broadcast silently.
    • The silhouette score handles singleton clusters and identical points correctly.
    • The RL improvement_rate had the wrong sign for negative rewards.
    • Sampling ranges that are too narrow now raise a clear error.
    • generate_seeds() no longer resets the global random state.
    • A broken TensorFlow install no longer breaks import seedhash.
    • The import-time pandas warning is gone.
  6. Docs. Corrected the cross-language claim in paper.md and the porting guide.
  7. Instructions. Updated the R range and R version docs, and fixed test commands that pointed at a tests/ folder that doesn't exist.

Breaking changes

  • R (not yet on CRAN):
    • seed_number and all generate_seeds() output change.
    • The default range of SeedHashGenerator$new() changes.
    • Fields are read-only.
    • Requires R ≥ 3.6.0.
  • Python:
    • Requires Python 3.8 or later.
    • Seeds and sampling output for valid inputs are unchanged; I checked 54 cases recorded before the changes.
    • Metric values change where they were wrong.

Testing

  • Python: 33 tests pass locally. 15 of them are new in test_correctness.py, and 12 of those fail on the old code.
  • R: 59 testthat expectations pass, and R CMD check reports Status OK (it reported 1 NOTE before).
  • Seed values for the same strings are pinned in both test suites, so a cross-language mismatch would fail CI.

🤖 Generated with Claude Code

melhzy and others added 7 commits September 30, 2026 07:48
- Derive seed_number as the MD5 digest of the UTF-8 input modulo 2^32, as
  the Python package does, so both languages give the same seed number.
- Default SeedHashGenerator$new() to [0, 2^31 - 1] like create_seedhash()
  and Python; the default range no longer fails in generate_seeds().
- Compute draws in doubles to avoid integer overflow to NA, return an
  integer vector, and support ranges wider than .Machine$integer.max.
- Validate bounds and count up front (whole numbers, no NA, not -2^31).
- Pin the RNG kind in generate_seeds() and restore the caller's state.
- Make fields read-only active bindings documented with @field.
- Add set_seed(), since set.seed(seed_number) fails above 2^31 - 1.
- Import R6 and digest in NAMESPACE, run the examples in R CMD check,
  and require R >= 3.6.0 for set.seed(sample.kind =).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The old script caught every error and only printed FAILED, so CI stayed
green while two of its tests failed. Move the tests to testthat, install
the package in CI and run them with stop_on_failure = TRUE. Add tests
for the seed values shared with Python, pinned generate_seeds() output,
RNG state handling, input validation and read-only fields.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Hierarchy nodes are keyed by seed value, so a child seed equal to an
existing seed overwrote that node's parent and could make a seed its own
ancestor; add_experiment_result() then looped forever. Keep the record
from a seed's first occurrence, guard the parent walk against cycles,
and copy the caller's metrics and metadata dicts.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- typing.Literal needs Python 3.8, so importing seedhash failed on 3.7.
- pyproject.toml's [project] table made setuptools ignore setup.py's
  extras_require, so the built wheel had no extras. Declare them in
  pyproject.toml and install the experiment extra in CI.
- torch.use_deterministic_algorithms(warn_only=True) needs torch 1.11.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Binary classification metrics no longer assume labels 0 and 1, and
  multi-class F1 is the mean of per-class F1 (both match scikit-learn).
- Flatten y_true/y_pred and reject size mismatches instead of silently
  broadcasting (n,) against (n, 1).
- Silhouette: singletons score 0 and identical points are excluded by
  index; results always have the same keys.
- RL improvement_rate divides by |early mean| and its windows no longer
  overlap for 10-19 episodes.
- Stratified and systematic sampling raise clear errors for ranges too
  narrow to sample; outputs for valid inputs are unchanged.
- generate_seeds() uses a private random.Random (same output) instead
  of reseeding the global random module.
- A broken torch/TensorFlow install no longer breaks import seedhash, TF
  uses enable_op_determinism(), and CUBLAS_WORKSPACE_CONFIG is kept if
  already set.
- Drop the import-time warning when pandas is missing.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
State that Python and R share the seed number but not the seed lists
from generate_seeds(), which come from each language's own RNG. Replace
the porting guide's advice to use the first 7 hex digits and -1e9..1e9
defaults, which caused the mismatch with Python.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- R docs still described the old -1e9..1e9 default, the "Range is too
  large" error and R >= 3.5.0; describe the 0..2^31-1 default, the full
  integer range, the -2^31 limit, set_seed() and R >= 3.6.0.
- Replace the porting guide's outdated R implementation and advice (7 hex
  digits, @description for fields) and mark its old fixes superseded.
- Test instructions pointed at a nonexistent tests/ directory; run
  pytest on the root test_*.py files and the R testthat suite instead.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 2, 2026 02:44

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

It is a broad, intentionally breaking cross-language change touching seed derivation, RNG-state handling, and numerical metrics whose R test suite could not be executed in this environment, so final human verification is warranted.

Review effort: Balanced
Findings: None

What changed in this PR

This PR applies a batch of correctness, packaging, and documentation fixes uncovered during a code review of the dual-language seedhash package (deterministic MD5-based seed generation). Its central goal is to make the R implementation produce the same seed_number as Python (MD5 digest of the UTF-8 input modulo 2³²), fix several latent Python bugs (an infinite loop on seed collisions, incorrect ML metrics, global-state side effects, fragile optional imports), modernize packaging/CI, and bring the many docs in line. The changes are intentionally breaking (R seed values, R default range, and several Python metric values change) but the package is pre-CRAN and the Python seed/sampling outputs for valid inputs are preserved.

Changes:

  • R: rewrite seed derivation to match Python, add read-only fields, set_seed(), overflow-safe generate_seeds() that restores caller RNG state, and a real testthat suite wired into CI.
  • Python: fix the add_experiment_result() infinite loop on repeated seeds, correct binary/multi-class/silhouette/RL metrics, add shape validation, stop mutating global random state, and harden optional framework imports.
  • Packaging/docs: require Python 3.8 / torch 1.11 / R 3.6.0, move extras to pyproject.toml, and update READMEs, the porting guide, and paper.md.
File Description
R/​R/​seedhash.R Core rewrite: Python-matching seed, read-only active bindings, set_seed(), RNG-state-preserving generate_seeds()
R/​tests/​testthat/​test-seedhash.R, testthat.R New testthat suite with pinned cross-language values
R/​tests/​test_seedhash.R Removed old always-exit-0 script
R/​man/​*.Rd, R/​NAMESPACE, R/​DESCRIPTION, R/​NEWS.md Regenerated docs, imports, deps (R ≥ 3.6.0, testthat)
R/​*.md, PYTHON_TO_R_GUIDE.md, GITHUB_INSTALL.md Updated cross-language and integer-range guidance
Python/​seedhash/​experiment.py Sampling validation, hierarchy cycle fix, metric corrections, dict copies
Python/​seedhash/​core.py Robust optional imports, private RNG in generate_seeds, TF/torch determinism tweaks
Python/​setup.py, pyproject.toml, requirements.txt Python 3.8+, torch 1.11+, extras in pyproject
test_correctness.py New regression tests (seeds, imports, metrics, hierarchy)
.github/​workflows/​tests.yml Install R package, run testthat, add correctness tests
README.md, Python/​README.md, paper.md, CONTRIBUTING.md, JOSS_*.md Doc/test-command updates

I verified the cross-language seed derivation by computation (R's last-8-hex construction equals Python's int(hex,16) % 2³²), reproduced the pinned Python seed/sampling values locally, and traced the metric, sampling-validation, and hierarchy-cycle logic by hand — all consistent. I did not find objective defects meeting the bar for a comment.

Files not reviewed (2)
  • R/man/SeedHashGenerator.Rd: Generated file
  • R/man/create_seedhash.Rd: Generated file

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@melhzy
melhzy merged commit 46df460 into main Oct 2, 2026
50 of 51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants