Repeaters

[cwjames1983]

Pulling in Jordan's latest modifications for fitting the repeaters distribution. He has recently pulled in much of zdmp, but not all of it I think

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[profxj]

Since this PR's head is a merge of zdmp into repeaters, the GitHub diff against main mostly repeats what I reviewed in #82. This review covers only the code unique to repeaters (git diff origin/zdmp...repeaters: 12 files, +293/−283). Line numbers refer to the branch head (d1fe7d7).

Good fixes in here that I want to call out: renaming os → obs in calc_log_posterior removes a real bug (the list shadowed the os module inside the function); lp += -PENALTY in minus_poisson_ps fixes the per-survey penalty being overwritten instead of accumulated; and the duplicate calc_max_dm() call in Survey.__init__ is gone.

Should be fixed before merge

1. zdm/data/Surveys/CHIME/CHIME_decbin_3_of_6.ecsv:260 — FRB 20190303A is given Z = -0.064. init_zs() classifies Z < 0 as unlocalised, so this repeater's host redshift is silently discarded and it is fit DM-only. The sibling edit in the same hunk (20180814A → 0.068) is positive, and papers/Repeaters/make_bestfit_plots.py:730 has the same source at z = 0.064, so this looks like a sign typo rather than a deliberate exclusion (which would be -1).

2. zdm/scripts/slurm/run_mcmc.slurm:32,46 — the active config references survey DSA_34 and parameter file data/MCMC/params2.json; neither exists in the repo (zdm/data/Surveys/ has only DSA.ecsv, zdm/data/MCMC/ only params.json). sbatch of the committed script fails before sampling starts.

3. zdm/scripts/run_slice.py:62 — plot_slice(...) is indented inside if not os.path.exists(out): os.makedirs(out), so plotting and the .npy saves are skipped whenever the output directory already exists (e.g. a re-run after a Slurm timeout). The multi-hour calc_slice loop then runs and writes nothing. Dedent the call out of the mkdir guard.

4. zdm/scripts/run_slice.py:53 — vals2 = [39.0] unconditionally forces lEmin = 39.0 (making the 'lEmin': 30.0 in param_dict dead) and bakes lEmin_2_39.0/ into every output path, even when lEmin is irrelevant or is itself the sliced parameter. Looks like leftover experiment scaffolding — either expose it as a CLI option or remove it.

Please confirm intent

5. zdm/data/Surveys/DSA.ecsv — MAX_LOC_DMEG changed -1 → 0 and 20221027A changed z = 0.5422 → -1. With MAX_LOC_DMEG == 0, init_zs() sets the cutoff to the minimum DMEG over rows with Z == -1 exactly (Z == -2 rows don't count). On the new file that minimum is 405.6 pc/cm³ (from 20221027A), so 25 of the 40 localised DSA FRBs have their redshifts overwritten to −1 and only 15 keep them. I assume the unbiased-threshold mechanism is intended (commit message: "use redshifts without bias"), but is 20221027A meant to be the FRB that sets the threshold? The DSA catalogue has a host at z = 0.5422. If it was meant to be excluded for another reason, -2 would keep it out of the threshold. Nothing in the file or code documents the −1 vs −2 distinction — a one-line comment in init_zs or the survey header would help.

6. zdm/data/Surveys/MeerTRAPcoherent.ecsv / MeerTRAPincoherent.ecsv headers — "NORM_FRB": 1, "TOBS": 317.5 was replaced by "MAX_LOC_DMEG": 1395 (coherent) and "NORM_FRB": 5 by "MAX_LOC_DMEG": -1 (incoherent). calc_log_posterior only adds a survey to the Poisson-count term if s.TOBS is not None (MCMC.py:267), so with --Pn both MeerTRAP surveys now silently contribute nothing to Pn. The commented-out no_Tobs/... survey lists in the slurm scripts suggest both variants were meant to exist — was dropping TOBS from the main files intentional?

7. zdm/iteration.py:2015 — with the += fix, the underflow penalty now matters more, and it is not monotonic: PENALTY = 100 (log10 units) is less severe than finite log10(pmf) values, which go down to about −308 before underflowing. Checked numerically for observed = 10: expected = 700 gives log10 pmf ≈ −282 (finite, counted in full); expected = 800 underflows and is penalised only −100. So minimise_const_only2 can prefer a wildly over-predicting constant over a merely bad one. scipy.stats.poisson.logpmf(os[i], r) / np.log(10) never underflows and removes the branch entirely.

Minor

8. zdm/MCMC.py:419-437 — Pool size now comes solely from SLURM_CPUS_PER_TASK with a fallback of 1, so on any non-Slurm machine the sampler silently runs single-process (previously Pool() used all cores). mcmc_runner already receives nthreads (and MCMC_wrap.py has -n/--nthreads) but never uses it — int(os.environ.get("SLURM_CPUS_PER_TASK", nthreads or mp.cpu_count())) would cover both cases.

9. zdm/MCMC.py:62-89 — profiled_calc_log_posterior has no call sites, and as written wouldn't work: cProfile.runctx(..., filename) returns None, so the profiled walker's log-prob would be None; and with the fork Pool, PROFILED_PID is None in every child, so every worker profiles every call and overwrites worker_<pid>.prof each evaluation. Either wire it up with a cProfile.Profile() object that returns the real value, or drop it.

10. zdm/data/Surveys/MeerTRAPcoherent.ecsv:33 — new row 20230808F uses a Unicode minus (U+2212) in XDec (−51:56:07.02). Harmless today because Gl/Gb are supplied, but it will break any future SkyCoord parse of that column and is inconsistent with every other row.

11. Branch state — repeaters is behind zdmp (merge-base 6bac170; zdmp head bccffb4 has the CI/dependency fixes from #82), so CI on this PR will keep failing at the pip install step until zdmp is merged in again.

---

Review prepared with Claude Code: multi-agent scan of the repeaters-only delta, with each finding verified against the branch (the DSA threshold and Poisson-underflow claims were recomputed numerically).