Consolidate rift_O4c (through 0.0.17.12) + master into rift_O4d — fork side of oshaughn#173 - #91
Merged
Merged
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>
…family O4c localized: container family runtime selection
…le-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
This was referenced Aug 13, 2026
…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
deployed
to
private-review-dispatch-rift
August 13, 2026 17:22 — with 
GitHub Actions
Active
GitHub Actions
Active
![interhost-cross-review-canary-v2[bot]](https://avatars.githubusercontent.com/u/274802661?s=60&v=4){kind=small}
oshaughnessy-junior
deployed
to
private-review-dispatch-rift
August 14, 2026 11:47 — with
GitHub Actions
Active
GitHub Actions
Active
oshaughnessy-junior
deployed
to
private-review-dispatch-rift
August 14, 2026 15:04 — with
GitHub Actions
Active
GitHub Actions
Active
oshaughnessy-junior
deployed
to
private-review-dispatch-rift
August 14, 2026 21:35 — with
GitHub Actions
Active
GitHub Actions
Active
oshaughnessy-junior
deployed
to
private-review-dispatch-rift
August 14, 2026 23:04 — with
GitHub Actions
Active
GitHub Actions
Active
oshaughnessy-junior
deployed
to
private-review-dispatch-rift
August 14, 2026 23:09 — with
GitHub Actions
Active
GitHub Actions
Active
oshaughnessy-junior
deployed
to
private-review-dispatch-rift
August 14, 2026 23:12 — with
GitHub Actions
Active
GitHub Actions
Active
oshaughnessy-junior
deployed
to
private-review-dispatch-rift
August 14, 2026 23:42 — with
GitHub Actions
Active
GitHub Actions
Active
oshaughnessy-junior
deployed
to
private-review-dispatch-rift
August 15, 2026 00:32 — with
GitHub Actions
Active
GitHub Actions
Active
oshaughnessy-junior
deployed
to
private-review-dispatch-rift
August 15, 2026 00:34 — with
GitHub Actions
Active
GitHub Actions
Active
oshaughn
deployed
to
private-review-dispatch-rift
August 15, 2026 00:52 — with
GitHub Actions
Active
GitHub Actions
Active
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.
Fork-side half of the O4c/master consolidation. Same branch and same commits as oshaughn#173 — that PR targets
oshaughn:rift_O4d, this one targetsoshaughnessy-junior:rift_O4d, so the headline pair stays equivalent.No collision risk between the two PRs
oshaughnessy-junior/rift_O4dis a strict ancestor of this branch (0 commits it has that the branch lacks, 39 the other way), so this is a fast-forward. Both PRs point at the same branch, so review changes pushed for one automatically apply to the other; the two cannot resolve a conflict differently.The one thing that would create a later collision is squash-merging either PR. That would rewrite the commits on one side only, and the next
origin↔juniorcross-merge would then re-present all of this history as new work. Please merge both with a merge commit (or fast-forward), not squash.Order does not matter, and neither does waiting.
Contents
Merges
junior/rift_O4d,origin/rift_O4c(through 0.0.17.12), andorigin/master, plus a port of the open O4c portfolio-driver fix (#172).Verified up front:
git.ligo.org/rapidpe-rift/riftis byte-identical tooshaughnon bothrift_O4candrift_O4d— 0 commits either way. There was no GitLab-only history to recover.The O4c gap is ~95% illusory: the container family, multi-GPU ILE fan-out, srate export fix, Virgo calibration convention and the
getenv='*'default had already reached O4d via this fork, in a more advanced form. Checked by per-file end-state diff direction rather than patch-id (which understates it, since the same content arrived by a different integration path): forcontainer_manifest.py,test_container_manifest.pyand themulti_gpudemo the O4d→O4c diff is pure deletion, zero insertions.Genuinely new, and the whole reason this PR exists:
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.--eccentricity-prior log_uniform(rapidpe-rift/rift!54, O4c 0.0.17.12), which landed on O4c mid-review and is pulled in here. See the defect note below.mcsampler.MCSampler. O4d already carried half of one of the four defects (comma splitting, but noNoneguard, no empty-member check, noelseclause), so that hunk was hand-resolved; the other three applied clean.⚠ 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)). The adjacentuniform_eccentricity_ln_priorhas the rightlog(ECC_MAX/ECC_MIN)normalization.This is upstream's bug on
rift_O4c, not a merge artifact, and it 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, so both lines converge. Happy to open that PR.Resolutions kept on the O4d side
setup.pystays0.0.18.0rc2— 0.0.17.x is the parallel O4c release line.calibration_reweighting.pykeepsast.literal_eval— O4c still has the bareeval.CHANGES.rstkeeps both release histories, O4d first.Testing
CI's test list (from
.github/workflows/ci.yml+.gitlab-ci.yml): 186 passed, 3 skipped. Merged-surface tests: 61 passed, including O4c'stest_asimov_bootstrap_source.pyandtest_container_manifest.pyrunning green against O4d's implementation. Both re-run after the 0.0.17.12 merge. The hand-merged--sampler-portfolioparsing checked against 7 cases.Nothing covers the eccentricity prior — no test on either branch exercises it, which is why the
np.lncrash shipped.All seven refs —
origin/{master,rift_O4c,rift_O4d},ligo/{master,rift_O4c,rift_O4d},junior/rift_O4d— report zero commits missing from this branch.Merging this does not close #172; that PR targets
rift_O4cand should still land there.🤖 Generated with Claude Code
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.