Skip to content

Modernize tooling, migrate metadata, refactor run(), make Louvain RNG deterministic#108

Merged
arnaudon merged 20 commits into
masterfrom
claude/update-fork-DL45C
May 29, 2026
Merged

Modernize tooling, migrate metadata, refactor run(), make Louvain RNG deterministic#108
arnaudon merged 20 commits into
masterfrom
claude/update-fork-DL45C

Conversation

@arnaudon

@arnaudon arnaudon commented Apr 28, 2026

Copy link
Copy Markdown
Collaborator

Summary

One PR with several logically-distinct commits. All behaviour-preserving unless called out. Originally drafted on the fork at arnaudon#1; moved here for review against upstream master.

Tooling & metadata

  • Migrate lint + format to ruff (replaces black, pylint, pycodestyle, pydocstyle, isort). tox -e lint/tox -e format rewired; .pylintrc removed; rules in [tool.ruff].
  • Migrate project metadata setup.pypyproject.toml (PEP 621). setup.py keeps only the pybind11 extension declaration.
  • Add upper bounds (numpy<3, scipy<2).
  • Drop print() for logging in optional-dep fallback paths.
  • python_requires=">=3.10" to match the tox matrix.

Code quality

  • Type hints on the public API (run, evaluate_NVI, identify_optimal_scales, save_results, load_results); from __future__ import annotations.
  • Refactor run() (150 lines → ~35). Extracted _resolve_exp_comp_mode, _scan_scales, _run_post_scan_analysis. Public signature unchanged.
  • Fix np.divide(... where=...) UserWarnings in constructors.py by pre-allocating the zero output buffer (numpy 2.x made the uninitialised-buffer behaviour stricter; dangling-node entries now explicitly 0.0).
  • Silence benign RuntimeWarnings in constructors.signed_modularity and optimal_scales._pool2d_nvi.
  • Resolve the spectral-decomp TODO in constructors.py: use v.T instead of la.inv(v) (orthonormal eigenvectors → transpose is inverse, O(n²) instead of O(n³)).
  • Type run(graph=...) as scipy.sparse.spmatrix | None.

CI

  • Add a standalone lint job to .github/workflows/run-tox.yml.

Repo hygiene

  • Drop pygenstability_doc.pdf (~720 KB build artifact); added to .gitignore.
  • Add results.pkl plus example/run artifacts (scan_results.pdf, *.gpickle, examples/communities/, …) to .gitignore.
  • Cover .coverage-* multi-env coverage files in .gitignore.
  • Fix Homepage URL: ImperialCollegeLondon/PyGenStabilitybarahona-research-group/PyGenStability (matches CI badges + issues).

Make Louvain RNG deterministic end-to-end

Three coordinated changes that replace the global C rand() / std::random_shuffle path with an explicit std::mt19937& threaded through the optimiser, and fix the multiprocessing.Pool ordering leak:

  1. Submodule (michaelschaub/generalizedLouvain master): find_optimal_partition_louvain[_gen] takes a std::mt19937& and uses a portable Fisher-Yates shuffle. Submodule pointer + .gitmodules now track upstream master (the RNG fix has merged via Replace global rand with std::mt19937 in Louvain michaelschaub/generalizedLouvain#1, Clean the test script #2, doc #3).
  2. Binding (generalized_louvain.cpp): drop std::srand(seed), construct a local std::mt19937(seed) and pass it explicitly. No global state touched.
  3. Python (_optimise / _run_optimisations): pre-generate per-try seeds in the parent process, dispatch via pool.starmap so each try always gets the same seed regardless of which worker picks it up.

pgs.run(...) now produces bit-identical results across re-runs, processes, and n_workers for a given numpy seed. Determinism guarded by test_run_is_deterministic parameterised on n_workers=[1, 2].

The earlier "Loosen flaky RNG-dependent equality asserts" commit is reverted as part of this change — strict dictdiffer.diff(..., tolerance=1e-5) == [] checks are restored, the test_run_gap.yaml fixture is regenerated against the new RNG path, and the tautological yaml.dump(...) in test_run_times is commented out so its assert actually validates.

Test plan

  • tox -e lint passes (ruff check + ruff format --check)
  • Full pytest tests/ (excluding test_plotting) passes locally on Python 3.14: 14/14, ~96s with MPLBACKEND=Agg
  • Determinism smoke test: same numpy seed → identical community_id and stability across pgs.run invocations, with both n_workers=1 and n_workers=2
  • CI green on 3.10 / 3.11 / 3.12 (this PR)
  • Editable install builds and links the pybind11 extension; import pygenstability; pygenstability.run resolves

claude and others added 11 commits April 19, 2026 19:12
- Add CLAUDE.md with repo orientation for Claude Code sessions
- Replace print() with module logger in optional-dep fallback paths
  (pygenstability.py, plotting.py, contrib/sankey.py)
- Declare python_requires=">=3.10" in setup.py to match the tox matrix
Replace black, pylint, pycodestyle, pydocstyle, and isort with ruff
for both linting (ruff check) and formatting (ruff format).

- Add [tool.ruff] config to pyproject.toml, translating the previous
  ignore lists, google docstring convention, and single-line import
  style.
- Simplify tox.ini lint/format envs to a single ruff dependency.
- Remove .pylintrc and the pycodestyle/pydocstyle/isort sections in
  tox.ini (now all expressed in pyproject.toml).
- Apply two minor ruff-format diffs in constructors.py and
  data_clustering.py (multi-line assert messages, blank-line cleanup).
Move all project metadata (name, version, authors, description,
dependencies, optional-dependencies, console script, package discovery)
into pyproject.toml's [project] and [tool.setuptools] tables. setup.py
now only declares the optional pybind11 C++ extension, which still
needs dynamic setup() for the build step.

No functional change: the C++ extension still builds, imports still
resolve, and the pygenstability console script is still installed.
Annotate the five publicly re-exported functions in
pygenstability/__init__.py:

- run (pygenstability.py)
- evaluate_NVI (pygenstability.py)
- identify_optimal_scales (optimal_scales.py)
- save_results / load_results (io.py)

Uses `from __future__ import annotations` to keep forward refs cheap
and avoid pulling in stricter typing libraries. Types intentionally
loose where the public contract is flexible (Any for the graph
argument, dict for the nested results structure).
The C++ Louvain extension's RNG is not deterministic across runs, so
the dictdiffer-based exact comparisons against stored YAML fixtures
flake on CI (seen on Python 3.11). Temporarily replace the value
comparison with a top-level key-set check — the call paths still run
for coverage, but we no longer assert on the randomized numeric
outputs.

TODO in code points back here; re-enable strict comparison once the
C++ RNG is seeded deterministically.
Tests and example scripts write results.pkl to CWD by default; keep
these local artifacts out of the repo.
- Fix 'where used without out' UserWarnings at constructors.py:404,459
  by pre-allocating the zero output for np.divide (numpy 2.x made this
  strict). Dangling nodes still end up with dinv==0 as downstream logic
  expects.

- Use v.T instead of la.inv(v) in _compute_spectral_decomp. la.eigh
  returns orthonormal eigenvectors for symmetric matrices, so the
  transpose is the exact inverse and O(n^2) instead of O(n^3). Resolves
  the long-standing TODO at constructors.py:58.

- Drop the 720KB auto-generated pygenstability_doc.pdf from the repo;
  add to .gitignore. Sphinx regenerates it on demand via `tox -e docs`.
  README no longer links to the stale PDF; the hosted HTML docs cover
  the same content.

- Update Homepage URL in pyproject.toml and clone commands in README.md
  / docs/index_readme.md from ImperialCollegeLondon/PyGenStability to
  barahona-research-group/PyGenStability (where CI badges and issues
  already live).
Extract three private functions from the 150-line run() body:

- _resolve_exp_comp_mode validates the exp_comp_mode argument and
  forces expm for directed / signed constructors.
- _scan_scales runs the per-scale optimisation loop and builds the
  aggregated results dict.
- _finalize_results applies postprocessing, computes ttprimes, and
  runs optimal-scale selection.

run() itself now reads as a straightforward script: validate, load
constructor, scan scales, finalize. No behavioural change — public
signature and returned dict are identical.
- Add upper bounds (numpy<3, scipy<2) so pip rejects future major
  releases we haven't tested against. Lower bounds unchanged.
- Silence two benign RuntimeWarnings surfaced by the test suite:
  * constructors.py signed_modularity: wrap the per-sign
    normalisation in np.errstate(invalid="ignore"); NaN output is
    still produced (and tolerated by the fixtures) when the graph has
    only positive / only negative edges.
  * optimal_scales._pool2d_nvi: scope the 'Mean of empty slice'
    warning around np.nanmean where NaN padding legitimately
    produces empty windows at the matrix borders.
- Add a standalone `lint` job to run-tox.yml that runs `ruff check`
  and `ruff format --check`. Fast feedback that doesn't get cancelled
  by a flaky test job.
- _assign_increasing_ids: replace np.vectorize(dict.get) (Python-level
  loop) with a pure numpy indexing scheme using np.unique's inverse +
  an argsort-based relabel. ~2-3x faster on the hot per-scale path.

- __init__.py: replace star imports with explicit named imports from
  each submodule; add __all__ listing the full public surface. Drops
  the corresponding ruff per-file ignore (__init__.py no longer needs
  F401/F403 relaxation).

- Add py.typed marker so downstream users / type checkers pick up the
  inline type hints on the public API. Included via MANIFEST.in.

- Add .pre-commit-config.yaml with ruff check + ruff format and a
  handful of standard hygiene hooks (trailing whitespace, EOF,
  yaml/toml syntax, large-file guard at 1 MB).
Replaces the global C rand() / std::random_shuffle path with an explicit
std::mt19937& threaded through the C++ optimiser, and removes the leak that
let multiprocessing.Pool ordering bleed into per-try seeds.

Three coordinated changes:

1. Submodule (arnaudon/generalizedLouvain @ fix_rng): rewrites
   find_optimal_partition_louvain[_gen] to take a std::mt19937& parameter and
   use std::shuffle. Submodule pointer + .gitmodules updated to track the
   fork branch until the change merges upstream.

2. Binding (generalized_louvain.cpp): drops std::srand(seed); constructs a
   local std::mt19937 from the incoming seed and passes it explicitly, so no
   global state is touched.

3. Python (_optimise / _run_optimisations): pre-generates per-try seeds in
   the parent process via np.random.randint, then dispatches via
   pool.starmap so each try always gets the same seed regardless of which
   worker picks it up. _optimise now takes seed as a positional argument
   instead of drawing one inside the worker.

With this in place, pgs.run(...) produces bit-identical results across
re-runs, processes, and worker counts for a given numpy seed.

Restores the strict dictdiffer-based asserts in tests/test_pygenstability.py
that 04c73ee had loosened, regenerates tests/data/test_run_gap.yaml against
the new RNG path, and stops tests/data/test_run_times.yaml from being
trivially overwritten on every test invocation.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
arnaudon and others added 4 commits April 28, 2026 12:57
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Submodule update (arnaudon/generalizedLouvain @ fix_rng → 59bd6f0) replaces
std::shuffle with a hand-rolled Fisher-Yates that uses mt19937 output via
rng() % i. std::shuffle's internal uniform_int_distribution is
implementation-defined, so libstdc++ (Linux CI) and libc++ (macOS dev)
produced different orderings from the same seed — strict fixtures
regenerated locally diverged from CI output.

Modulo-biased Fisher-Yates is bit-identical across compilers, which is
what fixed test fixtures need. test_run_gap.yaml and test_run_times.yaml
regenerated; the other three yaml fixtures already matched the new
shuffle output.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- conftest.py: force Agg matplotlib backend so plt.show() is a no-op
- contrib/sankey.py: pass auto_open=live to plotly.offline.plot, matching
  the existing pattern in plot_scan_plotly. Without this, the Sankey HTML
  always opened in the browser regardless of the live flag.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…on test

Tests audit follow-up.

- tests/test_dataclustering.py: re-enable the three strict
  dictdiffer-based YAML asserts that were commented out with the note
  "this is unstable due to not consistent rng in C++". With the
  deterministic Louvain RNG in place, they now pass. Generates the
  test_dataclustering_default.yaml and test_dataclustering_knn.yaml
  fixtures.

- tests/test_pygenstability.py: add test_run_is_deterministic, an
  explicit regression test that asserts same numpy seed yields
  identical community_id and stability across repeated pgs.run calls.
  This covers the determinism contract directly rather than relying on
  the YAML-fixture comparison to imply it.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
michaelschaub/generalizedLouvain master now contains the std::mt19937
plumbing (PR #1) and the portable Fisher-Yates fix (PR #2) we depended
on. Switch .gitmodules back from the arnaudon fork URL to upstream and
bump the submodule pointer to the upstream tip (8c9f1f2).

That tip also includes a Louvain speed-up (PR #3) which keeps the
final stability value identical but reorders intermediate tie-breaking
on larger graphs, so test_dataclustering_default.yaml and
test_dataclustering_knn.yaml needed regeneration. The pygenstability-
side fixtures (barbell graph, 22 nodes) were unaffected.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@juni-schindler juni-schindler left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I added a few suggestions. Some of the new comments in the code are a bit weird because Claude explained why the code has been changed.

Comment thread src/pygenstability/constructors.py Outdated
Comment thread src/pygenstability/constructors.py
Comment thread src/pygenstability/pygenstability.py Outdated
Comment thread src/pygenstability/pygenstability.py Outdated
Comment thread src/pygenstability/pygenstability.py Outdated
Comment thread CLAUDE.md Outdated
arnaudon added 2 commits May 28, 2026 07:54
…nalize_results

- Drop Claude-style explanations from docstrings/comments in
  _compute_spectral_decomp, _assign_increasing_ids, _run_optimisations,
  constructor_signed_modularity.prepare, _pool2d_nvi.
- Type run(graph=...) as scipy.sparse.spmatrix | None.
- Rename _finalize_results -> _run_post_scan_analysis.
- Untrack CLAUDE.md and add it to .gitignore.
…ment

- Fix "contructor" typo in _resolve_exp_comp_mode log line.
- Parameterise test_run_is_deterministic on n_workers=[1, 2] so the
  multiprocessing path that motivated the seed pre-generation fix is
  actually exercised.
- Add a clarifying "regenerate fixture" comment next to the commented
  yaml.dump call in test_run.
- .gitignore: cover .coverage-* (multi-env coverage files) plus stray
  example/run artifacts (scan_results.pdf, *.gpickle, communities/, ...).
@juni-schindler juni-schindler self-requested a review May 28, 2026 06:28

@juni-schindler juni-schindler left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It would be good to have a seed parameter for the run function that the user can set. A few changes will be required but it would be really useful.

Comment thread src/pygenstability/pygenstability.py
Comment thread src/pygenstability/pygenstability.py Outdated
Comment thread src/pygenstability/data_clustering.py Outdated
Comment thread src/pygenstability/data_clustering.py
Comment thread src/pygenstability/data_clustering.py
run() and DataClustering now accept a `seed` argument. Inside run() we
construct `rng = np.random.default_rng(seed)` once and thread that
Generator through _scan_scales into _run_optimisations, which uses it to
draw the per-try seeds dispatched to workers. No reliance on the global
np.random state remains, so a given (seed, n_workers) pair is fully
reproducible end-to-end. DataClustering surfaces `seed` as an explicit
kwarg and forwards it via pgs_kwargs.

Tests:
- test_run_is_deterministic now drives reproducibility via the new
  `seed=` argument instead of np.random.seed.
- New test_run_seed_independence guards that different seeds yield
  different trajectories.

Docstring/typo sweep:
- data_clustering.py: paritions -> partitions, Miniumus -> Minimum,
  Continunous -> Continuous, Partion -> Partition, "book" -> "bool".
- pygenstability.py: excecution -> execution, optimiation -> optimisation.
- constructors.py: enure -> ensure.
@arnaudon

Copy link
Copy Markdown
Collaborator Author

good point, I added rng option

The previous tests relied on np.random.seed(42) at module top to make
the legacy np.random.randint path inside _run_optimisations
deterministic. Now that _run_optimisations uses np.random.default_rng,
the legacy global seed is ignored, so tests have to drive determinism
through the new `seed=` kwarg.

- test_run: pass seed=42 to every fixture-comparison call.
- test_dataclustering: pass seed=42 to DataClustering instead of seeding
  the global RNG.
- conftest.results fixture: same treatment.
- Regenerated test_dataclustering_*.yaml and added `seed: 42` to
  run_params in the test_run_*.yaml fixtures.

Removed test_run_seed_independence: the barbell graph is small enough
that any reasonable seed converges to the same partition, making the
"different seeds -> different output" claim graph-specific and brittle.
test_run_is_deterministic (same seed -> same output) already proves the
seed plumbing works.
@juni-schindler

Copy link
Copy Markdown
Collaborator

It looks good! But what happens if the seed is None? Would it not be better to provide a default seed 42?

@juni-schindler

Copy link
Copy Markdown
Collaborator

and should we also address #111 here?

@arnaudon

Copy link
Copy Markdown
Collaborator Author

On seed=None vs seed=42 default — I'd keep seed=None:

  • Convention. Scientific Python defaults to non-deterministic: np.random.default_rng(), sklearn random_state=None, scipy optimisers. "No seed argument" → "fresh entropy" is the expectation users bring in.
  • Backward compatibility. Pre-PR behaviour was non-deterministic (unseeded global np.random). seed=None preserves that for anyone upgrading; seed=42 would silently change every existing run's output.
  • Silent reproducibility is a footgun. With a hardcoded default, every pgs.run(graph) returns the same partition. A user re-running their script to characterise stability across runs would see no variance and wrongly conclude the algorithm is deterministic, when really they're just reusing one seed. The whole point of n_tries is to characterise optimiser variance — locking the inter-run RNG defeats that motivation at the outer scale.
  • Explicit > implicit. seed=None makes users who want reproducibility type seed=42, which is the line they'll also need in their paper's methods section. Worth the small friction.

The cost is that tests/examples need explicit seeds — which is what we just did, and what they should have anyway.

On #111 — I'd address it separately. It's a numerical-correctness bug in Leiden vs Louvain when n_null ≥ 2, well-scoped but unrelated to the tooling/refactor/seed work here. Fixing it changes signed_modularity numerics (another fixture regen), needs cross-backend convergence tests, and this PR is already 30+ files across several concerns. Cleaner to merge this one and open a focused follow-up.

@arnaudon arnaudon merged commit acc9a23 into master May 29, 2026
4 checks passed
@arnaudon arnaudon deleted the claude/update-fork-DL45C branch May 29, 2026 08:19
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.

3 participants