Skip to content

Repo cleanup: fix seed bug, portable paths, docs - #1

Open
trvrb wants to merge 5 commits into
mainfrom
cleanup/docs-and-fixes
Open

Repo cleanup: fix seed bug, portable paths, docs#1
trvrb wants to merge 5 commits into
mainfrom
cleanup/docs-and-fixes

Conversation

@trvrb

@trvrb trvrb commented Jun 26, 2026

Copy link
Copy Markdown
Member

Via Claude

Summary

Repository cleanup pass — no scientific reimplementation. Fixes a few high-impact bugs, makes the repo reproducible and navigable, and writes real documentation. No pipelines were re-run (msprime/R are cluster-scale and unavailable locally), so results-changing fixes are made in code and the now-stale committed outputs are flagged, not deleted. See KNOWN_ISSUES.md.

Fixes

  • Simulation seed bug (sweep/base/make_dfs.ipynb): sim() read a module-level seed = 50 for every simulation, so the per-combo seed from sim_sweep_parallel() was ignored (no independent draws across the grid). Now threaded run_cell → simulate_one → sim(), matching the already-correct make_dfs_5reps.ipynb.
  • Hard-coded /home/nahmed/... paths: all active setwd()/root.dir/absolute read.csv() in the .Rmd files now use here::here(...); commented ones (pointing at a non-existent sweep_height_long/) tidied.
  • Dead code: removed migration/sweep/control/all.py (broken early-draft Snakefile — Python inside shell: blocks, mismatched I/O).
  • Control MRCA-height formula unified across ctrl/Snakefile and snake/Snakefile to (distance/2)/mu (comparable to t.time(u) on the sim/inf side); the old (total_branch_length - distance)/mu was not.

Reproducibility + docs

  • New environment.yml, install_packages.R, .gitignore.
  • Rewrote top-level / sweep/ / migration/ READMEs (fixed dead stagger/island and empty () links); added sub-READMEs for control/, merge_trees/, clades/.
  • New KNOWN_ISSUES.md consolidating fixed vs. open caveats (stale outputs to regenerate; confounded GAIA-vs-DTA comparison; sweep/ vs in_terminal/ divergence; unseeded sample counts; perfect-separation in logistic fits; etc.).
  • Added overview/known-issues cells to the core notebooks.

Notes for review

  • The make_dfs.ipynb diff is large because NotebookEdit normalized the 3 edited cells' source to a single string and reset their execution_count; no cells/outputs were lost and the notebook validates.
  • The all.py deletion landed in the scaffolding commit (it was pre-staged via git rm); functionally fine.

Test plan

  • All notebooks parse as valid JSON
  • No active setwd/root.dir//home/nahmed paths remain
  • environment.yml is valid YAML; all README relative links resolve
  • Reviewer: regenerate sweeps on the cluster and confirm the unified height formula before trusting control outputs

🤖 Generated with Claude Code

trvrb and others added 5 commits June 25, 2026 17:06
No environment spec existed. environment.yml pins the Python side (msprime,
tsinfer, tsdate, tskit, treetime, augur, bio2zarr); install_packages.R covers
the R/GAIA + plotting side; .gitignore excludes generated pipeline artifacts.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
sweep/base/make_dfs.ipynb: sim() read a module-level seed=50 for every
simulation, so the per-combo seed from sim_sweep_parallel() was ignored. Thread
seed through run_cell -> simulate_one -> sim(), matching make_dfs_5reps.ipynb.

Add top overview/known-issues markdown cells to make_dfs, make_dfs_5reps, and
make_geo_dfs documenting purpose, params, outputs, and stale-output caveats.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replace active setwd()/root.dir/absolute read.csv() calls with here::here(...)
so the .Rmd files knit from any checkout, and tidy the commented ones (which
pointed at a non-existent sweep_height_long/ dir).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Change snake/Snakefile's height to (distance/2)/mu to match ctrl/Snakefile and
t.time(u) on the sim/inf side; the old (total_branch_length - distance)/mu was
not comparable, since total_branch_length() is the sum of all branches, not tree
height. (control/all.py, a broken superseded Snakefile, was also removed.)

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Rewrite the top-level, sweep/, and migration/ READMEs (project motivation,
pipelines, params, setup) and fix their dead links (stagger/island dirs that
don't exist, empty () links). Add sub-READMEs for control/, merge_trees/, and
clades/, and a KNOWN_ISSUES.md consolidating fixed vs open caveats (stale
outputs, gene-conversion follow-up, confounded GAIA-vs-DTA comparison,
sweep-vs-in_terminal divergence, height formula, etc).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@trvrb

trvrb commented Jun 26, 2026

Copy link
Copy Markdown
Member Author

@nmmahmed --- take this or leave it as you see fit. This was me pointing Claude Code at your repo and asking it to clean things up and to document.

Reviewing Claude's PR, things I'd flag:

  • you really should always have a .gitignore file, it makes your life easier
  • you should have things like environment.yml for reproducibility, it's for you and others (see https://github.com/FredHutch/tfcb_2021/tree/main/lectures/lecture02)
  • you should always use relative paths, this is necessary for reproducibility, but helps you move between computers or between local computer and cluster (see also lecture02)
  • you should try to document the repo through README.md's (see also lecture02)

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.

1 participant