Consolidate rift_O4c + master + junior/rift_O4d into rift_O4d (and port the open O4c portfolio-driver fix) - #173
Open
oshaughnessy-junior wants to merge 50 commits into
Conversation
Point release for 0.0.17.7 See merge request rapidpe-rift/rift!48
…opt-in) ALTERNATIVE to the runtime-wrapper approach (branch rift_O4d_osg_runtime_container_select): use HTCondor's container universe with container_image = $$([...]) instead of MY.SingularityImage = ifThenElse(...). Why it works on OSG: MY.SingularityImage=ifThenElse(...) is an execute-side ClassAd expression that OSPool glidein pilots read as a LITERAL string and hold the job on. container_image with a $$() token is resolved by HTCondor via match-time machine-ad substitution (in the schedd, against the matched machine ad) BEFORE the job reaches the EP, so the pilot only ever sees a literal image URL. $$ in container_image is HTCondor's *documented* mechanism for selecting a container image by GPU CUDA capability, and container universe is the current OSPool-standard (it deprecated +SingularityImage); osdf:// container images are supported and OSDF-cached; GPU access is automatic under request_gpus (no --nv needed). The same path also works on the CIT-local pool, so this unifies both pools (vs the ifThenElse path which is CIT-local-only). - container_manifest.build_container_image_select(manifest): returns the $$([ ifThenElse(attr =?= undefined, <fallback img>, <ifThenElse selector>) ]) value. Image branches are the manifest images VERBATIM (osdf URL fetched by container universe, or cvmfs/local path in place) -- not a ./basename rewrite. The =?= undefined guard makes a CPU-only / non-advertising slot fall to the fallback image instead of an undefined $$() that would hold the job. - write_ILE_sub_simple: when RIFT_CONTAINER_UNIVERSE is set (and a family manifest + use_singularity), set universe=container, emit container_image = the $$() selector, and drop MY.SingularityImage / MY.SingularityBindCVMFS / the $$() transfer token (container universe transfers the image itself). The require_gpus floor is still applied. Default (env unset) behavior is unchanged: the existing ifThenElse MY.SingularityImage path for CIT-local runs. Tests: container_image select expression (undefined-safe, verbatim osdf URLs, fallback) and integration (universe=container, container_image=$$([...]), no MY.SingularityImage / no transfer token, floor present). Existing CIT-local and single-sif tests unchanged. Trade-off vs the wrapper branch: this is much smaller and uses native/documented HTCondor machinery, but relies on the matched slot advertising the capability attribute at match time; the wrapper detects the real GPU at job start instead. ILE-only for now (CIP/PSD/calibration still use the ifThenElse path). Open item to confirm on a real OSG GPU job: cvmfs bind + capability advertisement coverage across OSPool sites. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ainer universe)
write_calpilot_sub still handed the raw SINGULARITY_RIFT_IMAGE value to
MY.SingularityImage, so a .yaml/.yml family MANIFEST reached condor as the image
path and the job failed (a manifest is not a .sif). The container-universe work
fixed write_ILE_sub_simple but never touched the CALPILOT writer, even though the
CALPILOT job runs ILE internally (GPU) and needs the same per-machine selection.
Mirror write_ILE_sub_simple exactly:
* detect a container manifest (is_container_manifest) and expand it;
* legacy (default): universe=vanilla, MY.SingularityImage = ifThenElse(...),
plus the selective $$() osdf transfer token and a require_gpus floor;
* container universe (opt-in RIFT_CONTAINER_UNIVERSE): universe=container,
container_image = $$([...]) (match-time, OSG-safe), no MY.SingularityImage /
SingularityBindCVMFS, image delivered via container_image (no transfer token).
A plain .sif / osdf:// value keeps the legacy single-image behavior unchanged.
Validated offline (pilot DAG build, OSG=1, family manifest) in both modes: the
generated CALPILOT.sub container_image is byte-identical to ILE.sub, and the
require_gpus floor is applied. test_container_manifest.py: 15/15 pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
When SINGULARITY_RIFT_IMAGE is a container-family MANIFEST (.yaml/.yml), the osdf:// image URLs live INSIDE the manifest, so the existing `'osdf:' in singularity_image` auto-detect (which force-sets use_oauth_files='scitokens' for single-image osdf runs) misses it. Result: no `use_oauth_services = scitokens` in the subs -> the execute point has no credential to fetch the selected container -> every ILE/CIP/CALPILOT job is held with "credential is required for osdf://...sif but was not discovered". Add a manifest-aware branch: if singularity_image is a container manifest, inspect its image URLs and pick the same credential the single-image path would (igwn+osdf -> 'igwn', osdf -> 'scitokens'). Pipeline-writer only (bin/), no container rebuild. Validated offline: a family-manifest pilot build now emits `use_oauth_services = scitokens` on ILE/ILE_extr/ILE_puff/CALPILOT/CIP/ CIP_0/CIP_worker0, matching the old working single-image subs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…CIP fix) A CPU-only job (CIP) requests no GPU, so it matches a slot that advertises NO GPU capability attribute. The per-machine container_image = $$([ ... capability ... ]) then has nothing to resolve against: the $$() substitution fails to expand and HTCondor HOLDS the job -> all CIPs lock up. Fix: when a job requests no GPU, do not emit a $$() capability selection at all; use a SINGLE fixed container (the manifest fallback, i.e. the CPU-safe image). - build_container_image_select(manifest, request_gpu=True): with request_gpu= False it returns the plain fallback image literal (no $$(), no ifThenElse). - write_ILE_sub_simple passes request_gpu through (GPU jobs keep the $$ selector; a no-GPU ILE would also collapse). - write_CIP_sub: wire container universe for CIP too (universe=container, container_image = fallback literal, no MY.SingularityImage / BindCVMFS / $$() transfer token). CIP is CPU-only so it always collapses to the single image; no require_gpus floor (unchanged). Also corrects the stale CIP comment that claimed an undefined capability "collapses to the fallback image" -- true-ish for the native ifThenElse, but false for $$(), which holds the job. Tests: build_container_image_select(request_gpu=False) -> bare fallback image; CIP integration (universe=container, container_image = single fallback literal, no MY.SingularityImage / no $$() token / no require_gpus). 17/17 pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…o-hold)
Folds in the undefined-safe guard (orig 8b9a0c5d, fix/manifest-cpu-fallback) and
unifies it with the container-universe collapse already in this branch.
build_singularity_image_expr and build_transfer_input_expr emitted a bare
ifThenElse/ternary over TARGET.GPUs_Capability with no guard for that attr being
undefined. A job that matches a slot with no capability attribute -- a CPU-only
CIP slot, OR an OSPool GPU site that doesn't advertise it -- makes every
`TARGET.attr >= N` undefined, so the whole $$([...]) token "cannot expand" and
HTCondor HOLDS the job ("Cannot expand $$ expression").
Add an `undefined_safe` option to _build_selector that wraps the selector in
`TARGET.attr =?= undefined ? fallback : <selector>` (ternary for the comma-free
transfer token; ifThenElse otherwise). Apply it to both legacy builders, and
refactor build_container_image_select to reuse it (DRY) instead of its own inline
guard. An undefined-capability match now yields the fallback (smallest, CPU-safe)
image on every path instead of an unresolvable $$().
This is the central no-hold guard for the LEGACY (non-container-universe) path,
complementing the deterministic build-time collapse this branch already does for
CPU-only jobs under container universe (CIP -> single fallback container).
NB: the osdf scitokens credential for manifest images is a separate fix already
on dev (3e18793; re-proposed in PR #11) -- not duplicated here.
Tests: legacy builders are undefined-safe; updated the two exact-string
expression tests to the guarded form. 18/18 pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…) + CIP single image Fixes the CIT-LOCAL hold wave (CITLOCAL_BREADCRUMB_gpus_capability_undefined_holds.md): ~45% of CIT GPU slots satisfy the per-GPU require_gpus floor (per-GPU `Capability` inside AvailableGPUs) yet do NOT advertise the machine-level rollup attr `GPUs_Capability` that the family `$$()`/`ifThenElse` selection reads. On those slots the selection "cannot expand" and the job HOLDS (presents as stuck / MachineAttrMachine0=undefined). Measured 621 undefined / 741 defined, spanning node*/aframe/mly (not one bad host). Correct fix = do NOT match undefined-capability slots (don't guess their image): - container_manifest.build_capability_defined_requirement(manifest) -> "TARGET.<attr> =!= undefined" (generic on capability_attr; no-op where every GPU slot advertises it). GPU family jobs (ILE, CALPILOT) append it to Requirements. The defined set still includes the cc12.0 Blackwell nodes, so the family's purpose (Blackwell vs older) is preserved. - REVERT the undefined-safe `=?= undefined -> fallback` guard added in the prior PR (now on dev). It is UNSAFE for GPU jobs: an undefined-capability slot could be a Blackwell that hard-fails on the cuda-11.8 fallback -- the exact failure the family exists to avoid. We must not match it, not guess an image. _build_selector / build_singularity_image_expr / build_transfer_input_expr / build_container_image_select are back to a bare selector (fail-loud: an unexcluded undefined slot HOLDS rather than silently running the wrong image). - CIP (CPU, no GPU) holds the same way -- there is no GPU capability at all. CIP needs no GPU/arch-specific image, so it now uses a SINGLE fixed container = the manifest fallback on BOTH paths: legacy MY.SingularityImage = "./<fallback>" (QUOTED; a bare path is a ClassAd parse error) + transfer just that image; container universe container_image = the fallback URL. New helper build_fallback_single_image(manifest) -> (runtime_path, transfer_url). NOTE: the osdf scitokens credential for manifest images (3e18793) is already on dev. The getenv True->* default (dag_utils_generic vs dag_utils) is a separate, related item the breadcrumb flags -- not addressed here. Tests: capability-defined requirement (+ attr override); fallback single image (cvmfs in place vs osdf transferred); selectors are NOT undefined-guarded; ILE (legacy + container universe) emit the Requirements exclusion; CIP legacy emits a single quoted fallback with no exclusion. 22/22 pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…TENV=false) dag_utils_generic.py defaulted default_getenv_value / default_getenv_osg_value to 'True', emitting `getenv = True`, which schedds with SUBMIT_ALLOW_GETENV=false (e.g. CIT) reject -> the DAG aborts. The newer dag_utils.py already defaults '*' (all-env, the modern form); bring generic in line (value-only change, file's own formatting preserved to minimize a later oshaughn/rift_O4d->rift merge conflict). Still overridable via RIFT_GETENV / RIFT_GETENV_OSG. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A single integrate_likelihood_extrinsic_batchmode invocation evaluates the
contiguous intrinsic-grid range [--event, --event+--n-events-to-analyze)
serially on ONE GPU. On clusters where whole multi-GPU nodes are reserved but
ILE only requests request_GPUs=1, the remaining GPUs sit idle (e.g. macrongroup
~100 points per job on a 4-GPU node uses 1/4 of the hardware).
ile_pre.sh now wraps the ILE executable in a small launcher that, when opted in,
splits that point range into N disjoint shards run concurrently -- one per GPU.
Each shard is pinned with CUDA_VISIBLE_DEVICES and given a distinct
--output-file prefix (<orig>.gpu<dev>), so the per-point output files
(<prefix>_<localidx>_.dat / .xml.gz) never collide. Downstream collection is
unaffected: util_ILEdagPostprocess.sh globs CME*.dat and util_CleanILE.py
de-duplicates by parameter value, not filename. The shards partition the range
exactly (sizes differ by <=1), so coverage is identical to the serial run; the
launcher's exit code is the first non-zero shard code, preserving condor
retry/hold behaviour (e.g. CUDA hard-fail 62).
Controlled by env var RIFT_ILE_GPU_FANOUT (propagated to jobs via getenv=*RIFT*):
unset / "0" / "1" -> no fan-out; the launcher exec()s the binary unchanged,
so default behaviour is byte-for-byte identical.
"auto" -> one shard per visible GPU (CUDA_VISIBLE_DEVICES, else
nvidia-smi); for a whole node held with request_GPUs=1.
<int N> -> up to N shards (capped by #GPUs and #points); the DAG
also requests request_GPUs=N and request_CPUs=N so
HTCondor assigns the devices.
Changes are mirrored in dag_utils.py and dag_utils_generic.py (each carries its
own copy of write_ILE_sub_simple). request_CPUs is threaded through the
singularity branch so the fan-out CPU count is not clobbered back to 1.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Make the fan-out usable through every front-end, including asimov (which builds
the DAG in a clean environment and cannot rely on RIFT_ILE_GPU_FANOUT being
exported in the submit shell):
- Bake the resolved fan-out value into the generated ile_pre.sh as
export RIFT_ILE_GPU_FANOUT="${RIFT_ILE_GPU_FANOUT:-N}", so the job needs NO
runtime environment (a runtime value still overrides). ile_invocation_shell()
now takes the value; ile_gpu_fanout_value() resolves it at build time. Mirrored
in dag_utils.py and dag_utils_generic.py.
- Add --ile-gpu-fanout to util_RIFT_pseudo_pipe.py and
create_event_parameter_pipeline_BasicIteration; both funnel it through
RIFT_ILE_GPU_FANOUT (pseudo_pipe runs BasicIteration via os.system, inheriting
the env), so request_GPUs/CPUs sizing and ile_pre.sh baking happen on one path.
Asimov needs no code change: a blueprint sets the value via
scheduler.environment variables: {RIFT_ILE_GPU_FANOUT: N} (rift.py copies it
into os.environ before running the pipeline) or
scheduler.pipeline: {ile-gpu-fanout: N} (-> CLI flag).
Demo: demo/rift/infra/multi_gpu/ (README, Makefile, CI ini, asimov blueprint +
frozen container-family pin, fake_ile stub).
- make smoke-local: builds a REAL ile_pre.sh from the shipped helper around a
stub ILE and runs it across this node's GPUs; asserts exact coverage, GPU
spread, distinct per-shard output prefixes. Runs anywhere (no cupy/condor).
- make build / make verify: builds a real pipeline run dir on the CI synthetic
data (singularity/OSG so ile_pre.sh is emitted) with --ile-gpu-fanout and
asserts ILE.sub gets request_GPUs=N/request_CPUs=N and ile_pre.sh bakes N.
Verified end-to-end: ILE.sub -> request_GPUs=4/request_CPUs=4, ile_pre.sh ->
RIFT_ILE_GPU_FANOUT:-4 wrapping the container ILE binary; default stays 1 (no-op).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add the two ways to request a VARIABLE number of GPUs (HTCondor's plain
request_GPUs is a single fixed count, so it cannot natively ask for "1 to N"):
- RIFT_ILE_GPU_FANOUT=auto-max-N (shared / partitionable-slot pools)
request_GPUs/request_CPUs become a ClassAd expression that asks for up to N of
the capability-matching GPUs available on the matched slot:
ifThenElse(countMatches(RequireGPUs,AvailableGPUs) >= N, N,
ifThenElse(... >= 1, ..., 1))
(same countMatches idiom RIFT already uses for cross-platform GPU matching).
ile_pre.sh bakes 'auto', so the launcher splits across exactly the 1..N GPUs
condor grants. Override the expression with RIFT_ILE_GPU_REQUEST_EXPR if your
pool exposes GPU counts under a different attribute.
- RIFT_ILE_GPU_FANOUT=all (dedicated / whole nodes you reserve)
keep request_GPUs=1 (matches a node with ANY GPU count) and have the launcher
enumerate EVERY physical GPU via nvidia-smi, ignoring CUDA_VISIBLE_DEVICES.
Launcher _devices(physical=True) drives this.
The runtime split was already adaptive (the launcher splits the point block across
however many GPUs it is handed); these add the matching request side. Implementation:
ile_gpu_fanout_count() -> ile_gpu_request() returning (request_gpus, request_cpus) as
an int OR a ClassAd expression; ile_gpu_fanout_value() maps auto-max-N -> baked 'auto'.
Mirrored in dag_utils.py and dag_utils_generic.py.
Verified: launcher splits across 1/2/3/4 granted GPUs (full coverage each); 'all'
uses all 4 physical even with CUDA_VISIBLE_DEVICES=0; generated ILE.sub carries the
adaptive expression for auto-max-4, request 1 for 'all', fixed N for N.
Demo: new `make requests` shows request_GPUs/CPUs + baked launcher for each mode;
README "Values -- fixed vs. adaptive" documents the hot-swap options, the
partitionable-slot requirement, and the cgroup/reservation caveats; blueprint shows
the adaptive variants.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…back Per deployment policy (whole nodes are reserved; partitionable GPU slots are not a sustainable long-term path), make the GPU multi-GPU policy default to 'all': - DEFAULT_ILE_GPU_FANOUT = 'all'. When RIFT_ILE_GPU_FANOUT is unset, a GPU ILE job keeps request_GPUs=1 (matching unchanged -- lands on any GPU node exactly as before) but the launcher enumerates EVERY physical GPU (nvidia-smi, ignoring CUDA_VISIBLE_DEVICES) and splits the ILE block across all of them. On a 1-GPU node this is a no-op; only multi-GPU nodes change. - Fallback to the old single-GPU run: RIFT_ILE_GPU_FANOUT=1 (or 'single'/'off', or --ile-gpu-fanout 1). Aliases handled in the resolver and the launcher. - Safety: the bake is gated on request_gpu, so a CPU-only ILE job always bakes '1' and never grabs the node's GPUs under the 'all' default. Implementation: shared _raw_ile_gpu_fanout() applies the default + single/off aliases; ile_gpu_fanout_value()/ile_gpu_request() build on it; write_ILE_sub_simple passes fanout=(ile_gpu_fanout_value() if request_gpu else '1'). Mirrored in dag_utils.py and dag_utils_generic.py. Note: HTCondor partitionable GPU slots DO work today (verified on the CIT pool: a 2-GPU partitionable slot carves per-GPU dynamic slots), so auto-max-N remains available, but it is no longer the recommended/default path. Demo updated: README "Default policy" + Values table (all=default, 1/single=fallback); `make requests` shows default vs fallback; blueprint defaults to no override. Verified: default bakes 'all' and runs across all 4 physical GPUs even with CVD=0; RIFT_ILE_GPU_FANOUT=1 runs single; CPU-only job bakes '1'; fixed N and auto-max-N unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ntainer-family O4c localized: container family runtime selection
…ltigpu-ile-fanout O4c localized: multi-GPU ILE fanout
BUG: make sure marginal log likelihood calculation uses all available values
Fix virgo convention See merge request rapidpe-rift/rift!50
…oubling
The time-marginalisation upsampling block treated
--srate-resample-time-marginalization as a boolean: whenever the requested
rate exceeded --srate it refined the internal time grid by a hardcoded
factor of two and discarded the requested value.
With the O4c production settings (--srate 4096,
--data-integration-window-half 0.075) the internal grid is
tvals = linspace(-0.075, 0.075, int(0.15*4096) = 614)
whose spacing is 0.15/613 s (4086.7 Hz - already ~0.2% coarser than 1/4096,
because linspace spans the closed interval with N points). Doubling that
gives an exported time resolution of 8173 Hz. Every O4c production RIFT run
requested 16384 Hz and exported at ~8.2 kHz instead; this was confirmed by
measuring the minimum spacing between distinct geocentre times in
extrinsic_posterior_samples.dat across all 70 production rundirs.
Three changes:
* derive the refinement factor from the requested rate;
* derive it from the actual grid spacing rather than fSample, so
ceil(requested/fSample) cannot land just short of the target;
* end the dense grid on tvals[-1] rather than tvals[-1] + deltaT/2, so the
cubic spline is no longer asked to extrapolate past its last knot.
The dense grid still contains every original node, so lnL at the original
times is unchanged - this is a strict refinement, not a re-derivation.
Adds test/test_srate_resample_time_marginalization.py, including a guard that
fails if the shipped block and the tested reference implementation drift
apart.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Bootstrapping RIFT from an existing PE result currently requires expressing
that result as an asimov dependency, so _find_posterior can scan `needs:` for
a pipeline publishing a 'samples' asset. That is awkward when the file is
simply known to be on disk, and it is fragile: only the gwdata pipeline
returns 'samples' as a single path to a PESummary metafile. The bilby
pipeline returns a *list* of raw bilby result files, h5py rejects it, and the
bare `except Exception: pass` swallows the error - leaving the run with no
bootstrap at all and no message.
Adds a new optional `scheduler: bootstrap file:`, naming the PESummary
metafile directly and skipping the dependency scan:
scheduler:
bootstrap upstream: True
bootstrap file: /path/{event}/…/pesummary/samples/posterior_samples.h5
dataset: <label> # optional; auto-derived when unambiguous
The string accepts {event}/<event> and {analysis}/<analysis>, and may contain
shell wildcards, in which case exactly one match is required. A setting that
does not resolve to exactly one existing file raises rather than falling back
to the dependency scan - an explicit request must not fail silently.
No existing key is renamed or changes meaning, and the default path is
untouched: with `bootstrap file` absent, _find_posterior behaves exactly as
before. The grid is still built by the pipeline, by the existing code path,
with the existing `bootstrap size` / `bootstrap coinc` / `bootstrap amplitude`
handling; only the way the input posterior is located changes. Nothing here
is specific to any one analysis - paths and labels stay in the asimov config.
Also hardens label auto-derivation, factored out as _dataset_label():
* an explicit `dataset:` is still returned without opening the metafile,
preserving the old `if "dataset" not in self.production.meta` short-circuit
so ledgers that pin a dataset keep building when the source file has moved;
* when deriving, a label must be a root group that actually contains
samples, so metadata entries are no longer mistaken for analyses;
* an ambiguous metafile raises instead of silently taking the first label;
* the old list.remove('version')/remove('history') raised ValueError on
metafiles lacking those groups, which the bare except then hid;
* a raw bilby result file (samples at the root, no analysis label) is
detected and reported as such, instead of surfacing as 'Unknown key in
file' from deep inside the PESummary reader.
The dependency scan is unchanged in behaviour but now logs why a candidate
was rejected instead of discarding the exception.
Separately, warn when an existing <analysis>_bootstrap.xml.gz is reused: the
build skips regeneration if that file exists, so re-running an analysis under
the same name silently ignores a changed bootstrap source.
Adds test/test_asimov_bootstrap_source.py (15 tests). Replayed against all 66
O4c production ledger entries: every pinned dataset is returned unchanged, and
auto-derivation reproduces the recorded value on every event.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Follow-up to the previous commit. That commit derived an integer refinement
factor from the requested rate measured against the internal grid spacing:
n_upsample = max(2, ceil(requested * deltaT_orig))
Because the internal grid is a closed-interval linspace whose spacing is
~1/fSample but not exactly (614 points over 0.15 s -> 4086.7 Hz, not 4096), an
integer factor lands at n/deltaT_orig, not the request: a 16384 request snapped
to 5 x 4086.7 = 20433 Hz (+25%), and 32768 to 36780 Hz (+12%). Higher than
before, but not the requested rate.
Step the output grid by EXACTLY 1/srate_resample instead:
dt_target = 1.0/opts.srate_resample_time_marginalization
n_dense = floor((tvals[-1]-tvals[0]) / dt_target) + 1
tvals_denser = tvals[0] + dt_target*arange(n_dense)
The requested rates are powers of two, so 1/srate is exactly representable in
float64 and consecutive exported times differ by exactly that step, to the bit
(16384 -> 16384.000000 Hz, 32768 -> 32768.000000 Hz). floor() keeps the grid
inside [tvals[0], tvals[-1]] so the spline still never extrapolates; at most one
step (<1/srate s, tens of us) is dropped at the far edge of the +-75 ms window,
where the time-marginalized likelihood is negligible.
Tests updated to assert EXACT recovery (spacing == 1/requested, to the bit) for
8192/16384/32768/65536, plus an output-times-on-grid quantisation check; the
earlier >= requested assertions would have passed at 20433 Hz. The drift guard
now requires the exact-step form and forbids the integer-factor one.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ILE: honour --srate-resample-time-marginalization instead of always doubling See merge request rapidpe-rift/rift!51
Better bootstrap : asimov See merge request rapidpe-rift/rift!53
…am-private-review-dispatch Add private automated review dispatch for junior RIFT PRs
…d a blast-radius rule Two consecutive integrator PRs each needed FOUR review rounds, and in both cases every finding was the same shape: * #63 (AV collapse reporting) -- three findings that were all "a status changes after the thing consuming it has already run". * #78 / #84 (L0 rescue warm seed) -- four findings that were all "an array or a count looks like the population but is a filtered or resampled subset of it": the seed read the fair-draw export resample; the reject gate differenced two differently-resampled lnZ values; the cap kept -inf-dominated rows in proportion (2 of 10 finite rows survived) and consumed the global RNG; and the capped estimator's LOGARITHM carried sampling error though its linear total was unbiased. Every one of those returns a plausible-looking number when wrong, and none of them crashes, so neither CI nor a green test suite nor a successful GPU run distinguishes them. A repeating shape is checkable, which is what this adds: * RIFT/integrators/REVIEW_CHECKLIST.md -- the cheap read-it-yourself pass. * .github/PULL_REQUEST_TEMPLATE.md -- the four always-cheap items, plus an evidence section that asks WHICH REGIME a measurement was taken in. * A pointer from TESTING.md, which owns the expensive shape-recovery gate. The two catch different classes and neither subsumes the other. The blast-radius rule: run the cheap list on everything, and additionally request a full code review when the diff touches RIFT/integrators/, the ILE entry points, factored_likelihood.py or the CUDA kernels, any evidence/weight/ESS/collapse gate, or any changed default that production inherits. THREE CORRECTIONS from review of the first draft, all of which were the checklist making the mistakes it warns about: * The _rvs invariant was stated UNCONDITIONALLY and is not. Every sampler rebinds only when bFairdraw and n_extr is not None and n_extr < len(retained); otherwise _rvs IS the retained set and reading it is correct. Stated wrong, the item would have sent reviewers to the wrong population -- the exact error it exists to prevent. * _warm_seed_reserve was implied to be a generic sampler facility. It exists on mcsamplerAdaptiveVolume and mcsamplerPortfolio ONLY; mcsampler, mcsamplerGPU and mcsamplerEnsemble have no equivalent, so cross-sampler code needs its own fallback. * The CI item ASSERTED a coverage state that #84 had already made false. It now states the rule (grep the workflow; routing drifts) and keeps the outage as dated history. Also qualified every pull-request reference with the remote: this checkout has both oshaughnessy-junior and oshaughn, where a bare #78 resolves to a DIFFERENT pull request. Docs only; no code paths change.
…w, --stat, defaults) All three are the checklist failing its own items. 1. INCOMPLETE ENUMERATION. mcsamplerNFlow implements the identical fair-draw rebind and has no _warm_seed_reserve, and was missing from both lists. A reader could reasonably have concluded the invariant did not apply to it -- which is the "do not infer from the sampler class" error the item itself warns about, committed by the item itself. Rather than fix the list and leave it to rot, the item now leads with the two greps that REGENERATE it and dates the snapshot: all six samplers rebind (mcsampler, mcsamplerGPU, mcsamplerEnsemble, mcsamplerNFlow, mcsamplerAdaptiveVolume, mcsamplerPortfolio); only AV and portfolio keep a reserve. 2. `git show --stat` CANNOT ESTABLISH WHAT THE ITEM CLAIMED. --stat gives filenames and line counts, so it catches an unexpected FILE but not an unrelated edit inside a file the author also touched -- which is the case that actually occurred here, twice, when `git add -A` swept another branch's uncommitted mcsamplerPortfolio.py work into a commit that legitimately touched that same file. Now: read the diff; use --stat for the file list only. 3. THE PRODUCTION-DEFAULTS TRIGGER WAS UNREACHABLE FROM THE TEMPLATE. The canonical rule in REVIEW_CHECKLIST.md section 4 requires a full review for any changed default production inherits, but the template gated its full-review section on integrator/ILE/likelihood/ evidence paths only, so a defaults-only PR could satisfy the template while skipping the review the rule requires. Added to the section heading, and the shape-recovery item under it is now scoped by "can this move a posterior" rather than by directory. Docs only; no code paths change.
…these only my hunks"
The previous round fixed this in .github/PULL_REQUEST_TEMPLATE.md and left the CANONICAL
statement untouched -- REVIEW_CHECKLIST.md section 1 still said "`git show --stat` before
pushing", and the template points AT that document, so the authoritative instruction was
still the wrong one. Fixing one instance of a defect and leaving its twin is the shape this
checklist exists to catch.
--stat cannot answer the question. It reports filenames and line counts, so it catches an
unexpected FILE while being blind to an unrelated edit INSIDE a file you also changed -- and
that is the case that actually occurred here, both times: the swept-in work was in
mcsamplerPortfolio.py and the commit legitimately touched mcsamplerPortfolio.py, so --stat
showed one expected filename and a slightly larger line count.
Nor is it a useful companion, which is what the previous wording made it ("--stat for the
file list, the diff for the contents"). `git show` already names every file in its diff
headers, so a preliminary --stat adds a step and an opportunity to stop early. Both
documents now say: read the diff (`git show HEAD`, or `git diff --cached` before committing).
--stat survives only in the explanation of why not to rely on it.
Docs only; no code paths change.
…klist review: cheap pre-merge checklist for sampler/estimator changes, and a blast-radius rule
…t two integrals The gate keeps the cold pass whenever its lnZ lands more than --sampler-l0-rescue- reject-dlnZ (0.5) above the warm pass's, on the reasoning that the cold pass had full support so a lower warm evidence means the seed missed mass. Two separate things made that comparison not a comparison. 1. THE TWO READINGS CAME FROM A RESAMPLE BUILT FOR EXPORT. Both were read from _rvs, which the fair draw has already replaced with min(n_extr, 1.5*eff_samp, 1.5*neff) rows taken WITH REPLACEMENT, proportional to weight. _lnZ_of_rvs forms logsumexp(w) - log(n), so rows drawn proportional to w cluster at the top of the weight distribution and the estimate lands near max(w) rather than mean(w) -- high by roughly log(n_retained/eff_samp). The passes are drawn at very different n and eff_samp: at rho_net 146.8 the cold reading is ONE row out of 1000 retained at eff_samp ~ 1 (about 7 nats high) against 5 rows at eff_samp ~ 8 for the warm pass. The gate was reading the difference between its own two subsample sizes as evidence of lost mass, discarding a warm pass at ESS 12.5-28.6 in favour of a collapsed cold pass at ESS 1.0-2.1 in 10 of 12 replicates. With both sides read from the retained set that drops to 3 of 12, on a like-for-like gap of 2.0-3.9 nats. 2. AND THE RETAINED-SET READING NEEDED ITS OWN DENOMINATOR (review). make_warm_seed_ reserve drops non-finite rows and may cap what survives, so the stored array is neither the draw set nor a uniform sample of it. Averaging over stored rows divides by the wrong n, overestimating by log(n_retained/n_finite) -- ~11 nats for a portfolio, whose _rvs holds every draw and whose finite fraction on a collapsed pass is ~1e-5. That error does NOT cancel in the gate, because the cold and warm passes have different finite fractions: it is the difference of two different-sized errors, which is the same class of defect one layer down. lnZ_from_reserve rebuilds the estimate on the original proposal-draw normalization, and collapses to the plain row average exactly when nothing was dropped and nothing was capped -- i.e. for AV, whose retained set is already all finite, so the measurements above are unaffected. The gate also refuses to compare across conventions: if the two passes did not produce the same kind of reading it falls back to reading BOTH from the fair-draw record -- the previous behaviour, which was at least self-consistent. This does not weaken what the gate is for. A seed that genuinely misses a mode still produces a lower warm evidence on a like-for-like comparison; what stops is rejecting on a difference that was never there. Tests: the fair-drawn reading is the high one and tracks log(n/eff_samp); lnZ divides by the draws made, not the finite survivors; it is unchanged when every row is finite; it accounts for the uniform cap; the error does not cancel between two passes at different finite fractions; and an end-to-end portfolio pass with underflowed draws reports a draw-normalized lnZ (asserting first that the target really underflowed).
…-reland Re-land PR #79: the L0 reject gate was comparing two different estimators
…erge-consolidation-030e3c
Nearly all of the O4c gap (container family, multi-GPU ILE fan-out, srate export fix, Virgo calibration convention, getenv default) had already landed in O4d independently via the junior fork, in a more advanced form: container_manifest.py, test_container_manifest.py and the multi_gpu demo are strict O4d supersets, so the add/add conflicts resolve to O4d. The one genuinely new item is the asimov explicit bootstrap source (rapidpe-rift/rift!53, O4c 0.0.17.11). asimov/rift.py merges cleanly as a strict superset of BOTH parents: O4c's _resolve_bootstrap_file / _dataset_label plus the broadened 'bootstrap upstream or bootstrap file' guard, on top of O4d's bootstrap-amplitude, grid-reuse safety, 'use global priors' override and scheduler-environment passthrough. Kept on the O4d side deliberately: - setup.py stays 0.0.18.0rc2; 0.0.17.x is the parallel O4c release line. - calibration_reweighting.py keeps ast.literal_eval (O4c still has eval). CHANGES.rst keeps both release histories, O4d first.
…ge-consolidation-030e3c
Eccentricity log-uniform prior changes See merge request rapidpe-rift/rift!54
…ng a sampler Driver-side half of making --sampler-method portfolio usable on rift_O4c. The module-side half -- guarding the unguarded entry-point plugin load, so one missing optional dependency does not make mcsamplerPortfolio unimportable -- is PR oshaughn#168's first commit (aaa8a6e) and is deliberately NOT duplicated here. Neither half is sufficient alone; oshaughn#168 does not touch this file. Four defects, all in the sampler-construction chain, all of the same kind: a request the driver cannot honour is answered with something other than an error. 1. `elif opts.sampler_method == "portfolio" and mcsampler_Portfolio_ok:` made the very next statement -- `if not(mcsampler_Portfolio_ok): raise` -- unreachable, and sent an unavailable portfolio down the chain to the terminal `else`, which prints " ILE: **original sampler** " and proceeds with the plain mcsampler.MCSampler constructed before the chain. A run that asked for the portfolio would have integrated with a different sampler. Drop the ok-flag from the test so the existing raise becomes reachable and does its job. 2. That terminal `else` then dereferences `mcsamplerPortfolio.known_pipelines` in its own diagnostic print -- but the name is only bound if the import succeeded, so the diagnostic for a failed import raised NameError itself. This is what '--sampler-method portfolio' actually produced on a torch-free container: NameError: name 'mcsamplerPortfolio' is not defined 3. The plugin-pipeline branch had the same unguarded dereference in its `elif` test. Gate it on mcsampler_Portfolio_ok. 4. --sampler-portfolio is action='append' while its help documents a comma-separated list, and the member loop had no else clause. So the documented invocation '--sampler-portfolio AV,GMM' arrived as the single member name "AV,GMM", matched no branch, and appended whatever `sampler` happened to hold -- the plain MCSampler from before the chain, or on later iterations the PREVIOUS member. The portfolio then ran with a member nobody asked for and died later and elsewhere with a misleading "no attribute 'draw_simplified'". Accept both spellings (and a mix), and make an unrecognized member name an error naming the known members. The committed demo harness already works around this by splitting on commas in shell before invoking the driver (extrinsic_collapse_demo/run_demo.sh:58), which is why the append form is the only one that had been exercised. Ported to rift_O4d (cherry-pick of the rift_O4c commit). O4d already carried HALF of defect 4 -- it splits on commas, but with no None guard, no empty-member check and no else clause -- so that hunk conflicted and was resolved by keeping O4d's comment and expression and adding the two missing guards. Defects 1, 2 and 3 were entirely absent from O4d and applied clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 3201b00)
oshaughnessy-junior
had a problem deploying
to
private-review-dispatch-rift-upstream
August 13, 2026 15:32 — with 
GitHub Actions
Failure
GitHub Actions
Failure
Second O4c pull in this branch: origin/rift_O4c advanced from 4a8703f to ddfc637 while the consolidation was in review. New content is the log-uniform eccentricity prior (rapidpe-rift/rift!54); O4d had no equivalent, so this is a genuine ingest rather than a duplicate. Same two resolutions as the first O4c merge: setup.py stays 0.0.18.0rc2 (0.0.17.x is the parallel O4c release line), and CHANGES.rst keeps both release histories with O4d first. KNOWN UPSTREAM DEFECT, carried across unmodified so the two lines do not fork: log_eccentricity_prior() calls np.ln, which does not exist in numpy, so --eccentricity-prior log_uniform raises AttributeError as soon as the prior is evaluated. The normalization is also wrong independently of that -- a log-uniform density on [ECC_MIN, ECC_MAX] is 1/(x*log(ECC_MAX/ECC_MIN)), not 1/(x*log(ECC_MAX-ECC_MIN)). Both should be fixed on rift_O4c, where the code originated, so O4c and O4d converge instead of diverging on this line.
oshaughnessy-junior
had a problem deploying
to
private-review-dispatch-rift-upstream
August 13, 2026 16:38 — with
GitHub Actions
Failure
GitHub Actions
Failure
…a test
--eccentricity-prior log_uniform (0.0.17.12, rapidpe-rift/rift!54) could never
have run: log_eccentricity_prior calls np.ln, which does not exist in numpy, so
evaluating the prior raises
AttributeError: module 'numpy' has no attribute 'ln'
Independently of that, the normalization was the uniform prior's, not this
one's. For a density uniform in ln(e) on [ECC_MIN, ECC_MAX],
\int_ECC_MIN^ECC_MAX dx / (x*C) = 1 => C = ln(ECC_MAX/ECC_MIN)
not ln(ECC_MAX-ECC_MIN). With the shipped ecc-min auto-correction of 0.001 and
ecc-max 0.4 the old constant is ln(0.399) = -0.918: negative, so the prior would
have been negative everywhere had it evaluated at all. The adjacent
uniform_eccentricity_ln_prior already had the right ln(ECC_MAX/ECC_MIN) form.
Why this shipped: CIP is a script, not an importable module, so none of its ~40
prior densities has ever had a test. test_cip_priors.py closes that. It does
not transcribe the priors -- a copied reference implementation drifts from the
shipped code and then tests nothing -- but extracts the real def blocks from the
CIP source with ast and execs them in a namespace holding numpy and the handful
of module-level constants they close over. The functions under test are
byte-identical to the ones CIP runs.
Two layers:
- test_prior_evaluates, over all 38 extracted priors: each must evaluate on
its support and return finite, non-negative, broadcastable densities. This
is the generic guard that catches an np.ln (a name that does not exist) in
any prior, including ones added later, with no new test to write.
- test_prior_is_normalized, over the 19 that claim a normalized density: each
is integrated numerically over its stated support and must come to 1. This
is what catches a wrong normalization constant, which raises nothing and
merely reweights the posterior. The measure is spelled out per prior ('x',
d(ln x), d(x^2)) rather than inferred, since choosing it wrongly is the bug
class under test. Priors documented in-source as unnormalized are listed
and excluded on purpose.
Plus two shape tests specific to the regression: e*p(e) constant (so p really
is log-uniform, independently of the constant), and that the flat and
log-uniform eccentricity priors are not the same function -- if a refactor
collapsed one onto the other, --eccentricity-prior would silently stop doing
anything.
Verified as a regression test: against the unfixed 0.0.17.12 CIP it fails 4/60,
all four naming log_eccentricity_prior; against the fix, 60 pass.
Two notes for review, deliberately NOT changed here:
- selection is on 'prior' appearing anywhere in the function name, not a
'_prior' suffix. The suffix rule silently skips s_component_zprior,
s_component_zprior_positive and both *volumetricprior densities.
- the test does not import lalsimutils (it parses p_R out of the source):
importing it pulls in LAL, whose default error handler calls abort(), which
turns any numerical complaint inside scipy.integrate.quad into a core dump
instead of a test failure.
Landed on rift_O4d together with the 0.0.17.12 merge that brought the defect
across, so O4c and O4d converge on this function rather than diverging. The
extraction finds the same 38 priors in O4d's CIP as in O4c's (identical name
sets), and all 60 tests pass here too.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
(cherry picked from commit 5a3c7a1)
oshaughnessy-junior
had a problem deploying
to
private-review-dispatch-rift-upstream
August 13, 2026 17:22 — with
GitHub Actions
Failure
GitHub Actions
Failure
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
3 participants
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.
Consolidates
rift_O4c(GitHub + GitLab),master, andoshaughnessy-junior/rift_O4dintorift_O4d, and ports the one open O4c fix that O4d still lacked.What was actually diverged
git.ligo.org/rapidpe-rift/riftandoshaughn/research-projects-RITare byte-identical on bothrift_O4candrift_O4d— zero commits either way. So there was no GitLab-only history to recover; everything merged there is already on GitHub.The 29-commit
rift_O4cgap is mostly an illusion. The container family, multi-GPU ILE fan-out, srate export fix, Virgo calibration convention and thegetenv='*'default had already landed in O4d independently via the junior fork, in a more advanced form. Verified file-by-file rather than by patch-id: forcontainer_manifest.py,test_container_manifest.pyand themulti_gpudemo the O4d→O4c diff is pure deletion, zero insertions — O4d is a strict superset.dag_utils_generic.pyis actually ahead, carrying an extraand not singularity_runtime_selectterm in the container-transfer guard that O4c lacks.rift_O4dligo/rift_O4c,ligo/rift_O4dorigin/rift_O4corigin/masterjunior/rift_O4dContents
junior/rift_O4d— L0 rescue reject-gate fix, portfolio warm-pass sample reserve, sampler/estimator pre-merge review checklist. Clean merge.origin/rift_O4c0.0.17.9→0.0.17.11 — the one new item is the asimov explicit bootstrap source (scheduler: bootstrap file:, rapidpe-rift/rift!53) plus its 204-line test.asimov/rift.pymerged as a strict superset of both parents (zero deletions relative to either side): O4c's_resolve_bootstrap_file/_dataset_labeland the broadened'bootstrap upstream' or 'bootstrap file'guard on top of O4d's bootstrap-amplitude, grid-reuse safety,use global priorsoverride and scheduler-environment passthrough.origin/master— private review-dispatch workflow (Add private automated review dispatch for junior RIFT PRs #167). The evidence-calculation fix (BUG: make sure marginal log likelihood calculation uses all available values #156, C. Talbot) was already patch-equivalent in O4d.Conflict resolutions
Eight conflicts. Six were add/add on files proven to be O4d supersets, resolved to O4d. The two judgement calls, both kept on the O4d side because O4c would have regressed them:
setup.pystays0.0.18.0rc2. 0.0.17.x is the parallel O4c release line, not O4d's.calibration_reweighting.pykeepsast.literal_eval. O4c still has the bareevalthere.CHANGES.rstkeeps both release histories, O4d first.The #172 port
O4d already carried half of defect 4 — it splits
--sampler-portfolioon commas, but with noNoneguard, no empty-member check and noelseclause. That hunk conflicted; resolved by keeping O4d's comment and expression and adding the two missing guards. Defects 1, 2 and 3 were entirely absent from O4d and applied clean:elif opts.sampler_method == "portfolio" and mcsampler_Portfolio_ok:made theraiseon the next line dead code and sent an unavailable portfolio to the terminalelse, which silently integrates with the plainmcsampler.MCSampler.elsedereferencedmcsamplerPortfolio.known_pipelinesin its own diagnostic, so a failed import raisedNameErrorfrom the error message on a torch-free container.elifhad the same unguarded dereference.samplerbound to its previous value and appended it silently.Testing
.github/workflows/ci.yml+.gitlab-ci.yml): 186 passed, 3 skipped.test_asimov_bootstrap_source.pyandtest_container_manifest.pyrunning green against O4d's implementation, an independent confirmation of the superset resolutions.None/empty parsing checked against 7 cases (None,[],['AV,GMM'],['AV','GMM'], mixed, whitespace,[',']).Two collection errors in
test/(factored_likelihood_test.py,integrators/test_mcsampler_rosenbrock.py) are pre-existing legacy-script issues in files untouched here, and neither is in CI's list.After this, all seven refs —
origin/{master,rift_O4c,rift_O4d},ligo/{master,rift_O4c,rift_O4d},junior/rift_O4d— report zero commits missing from the branch.Note on the other open O4c PRs
#168, #169 and #171 are O4d→O4c backports; their content is already in O4d. Only #172 was missing, and it is included here. Merging this does not close #172 — that PR targets
rift_O4cand should still land there.🤖 Generated with Claude Code
Update: also merged O4c 0.0.17.12, and paired with a fork-side PR
Paired PR: oshaughnessy-junior#91 targets
oshaughnessy-junior:rift_O4dfrom the same branch and same commits, so the headline pair stays equivalent. Review changes pushed for one apply to both; they cannot resolve differently. Order does not matter.Please merge both with a merge commit (or fast-forward), NOT squash. Squashing either side rewrites the commits on that side only, and the next
origin<->juniorcross-merge would re-present all of this history as new work.origin/rift_O4cadvanced from4a8703f3toddfc637fduring review; that has been merged in too. New content is the log-uniform eccentricity prior (--eccentricity-prior log_uniform, rapidpe-rift/rift!54). O4d had no equivalent, so it is a genuine ingest. Same two resolutions as before (setup.pystays0.0.18.0rc2; CHANGES.rst keeps both histories). All tests re-run after it: 186 passed / 3 skipped, plus 61 on the merged surface.⚠ Known upstream defect carried across unmodified
log_eccentricity_prior()inutil_ConstructIntrinsicPosterior_GenericCoordinates.pycallsnp.ln, which does not exist in numpy, so--eccentricity-prior log_uniformraisesAttributeErroras soon as the prior is evaluated. The normalization is independently wrong: a log-uniform density on[ECC_MIN, ECC_MAX]is1/(x*log(ECC_MAX/ECC_MIN)), not1/(x*log(ECC_MAX-ECC_MIN)).This is upstream's bug on
rift_O4c, not a merge artifact, and is deliberately not patched here — fixing it in the merge would fork the exact line this consolidation exists to keep collision-free. It should be fixed onrift_O4c, where it originated. No test on either branch exercises this prior, which is why it shipped.Update: the eccentricity-prior defect is now FIXED here, not just flagged
The
np.lncrash and the wrong normalization described above are fixed on this branch, together with the first test suite the CIP priors have ever had. Same commit is up as oshaughn#174 againstrift_O4c, where the code originated, so the two lines converge on this function instead of diverging.log_eccentricity_priornow usesnp.log(ECC_MAX/ECC_MIN). The old constantln(ECC_MAX-ECC_MIN)isln(0.399) = -0.918for the shipped 0.001/0.4 defaults — negative, so the prior would have been negative everywhere had it evaluated at all.test_cip_priors.pyextracts the shippeddefblocks from CIP withastrather than transcribing them (a copied reference implementation drifts and then tests nothing), so the functions under test are byte-identical to the ones CIP runs, with no argparse or I/O executed. All 38 priors must evaluate finite and non-negative; the 19 claiming a normalized density must integrate to 1 against an explicitly-stated measure (dx,d(ln x),d(x²)).log_eccentricity_prior; 60 passed with the fix. The extraction finds the same 38 priors in O4d's CIP as in O4c's (identical name sets).Test totals for this branch are now 246 passed / 3 skipped on the CI list plus the new prior suite, and 61 on the merged surface.
Two things found while writing the test and deliberately left alone, flagged in #174 for a maintainer:
s_component_volumetricpriorintegrates toR/9rather than 1 (excluded as unnormalized; I have not verified whether that factor is intended), andutil_ConstructIntrinsicPosterior_GenericCoordinates.py:1007containss_compone+nt_zprior_positive, a typo that raisesNameErrorwhenever--aligned-prior alignedspin-zprior-positiveis used.