Skip to content

Preserve generic GWSignal options through production waveform paths - #217

Merged
oshaughn merged 7 commits into
oshaughn:rift_O4cfrom
oshaughnessy-junior:codex/gwsignal-options-o4c
Oct 12, 2026
Merged

oshaughn merged 7 commits into
oshaughn:rift_O4cfrom
oshaughnessy-junior:codex/gwsignal-options-o4c

Conversation

@oshaughnessy-junior

@oshaughnessy-junior oshaughnessy-junior commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

GWSignal reconstructed its input dictionary using only lmax_nyquist from extra kwargs. Production --internal-waveform-extra-kwargs requests for enable_antisymmetric_modes and antisymmetric_modes_hm were parsed and logged by ILE but dropped before waveform generation.

This forwards arbitrary backend options in all three GWSignal entry points, using a separate explicit backend dictionary through ILE and calibration reweighting. New options, nested dictionaries, false/zero values and opaque backend objects survive without a waveform-option whitelist. Explicit options take precedence over legacy kwargs; physical source-parameter overrides raise to prevent disagreement with likelihood parameters and waveform metadata. Missing GWSignal and incompatible explicit LALSuite dictionaries fail clearly.

The production Asimov → INI → pseudo-pipe → BasicIteration → Condor path also needed repairs: shell quoting now preserves nested strings/lists/backslashes, manual extra arguments accept lists or strings, both waveform dictionary namespaces are emitted, and both Condor DAG writers share the same argument encoder. Calibration reweighting preserves the selected backend and option namespace.

Validation:

  • 207 local regression cases passed with real LAL/GWSignal/RIFT imports, including all three wrapper entry points, actual likelihood/calibration forwarding, false/zero/nested and opaque options, precedence/conflicts, double quoting, missing dependencies, and backend error propagation.
  • Actual Liquid rendering and actual Condor submit-file writing verify both dictionary namespaces and escaped values.
  • A real BasicIteration subprocess generated a DAG and ILE submission from rendered Asimov options and a one-point XML grid.
  • A full top-level pseudo-pipe → helper → BasicIteration build using repository GW150914 CI fixtures generated an iterative DAG, extrinsic ILE and calibration-reweighting submissions. All three preserve the exact nested/quoted/backslashed dictionary. Local calibration/PSD stand-ins and a never-executed Bilby executable-resolution stub are build-only fixtures; no scientific workers were launched.
  • CIT production-container execution of the real ILE with production-style CLI reached generation of 21 modes with both requested antisymmetric flags. Independent adversarial review checked the actual driver, beyond AST extraction.
  • Actual HTCondor parsed the generated files and 36 tiny argv-recorder jobs exited normally with exact worker arguments, including complete-DAG-macro checks of the final full-build ILE, ILE_extr and calibration submissions from both branches. 39 tiny jobs were launched in total; three preliminary runs with incomplete test macro setup are excluded from the exact-argv validation count. These jobs recorded argv rather than performing parameter estimation.
  • Six generated Slurm scripts were actually executed with argv recorders; explicit Glue and HTCondor emission were also checked.

The regression harness extracts narrow executable option-handling statements where importing the full driver requires detector data; the full-driver production-container and scheduler checks above cover those operational boundaries separately.

Historical expression-dictionary compatibility is preserved at the existing pseudo-pipe and O4c calibration parser sites. Regression checks cover direct and double-quoted 1/10 expressions, nested dictionaries and booleans through the actual calibration source and GWSignal generator boundary.

@oshaughnessy-junior
oshaughnessy-junior deployed to private-review-dispatch-rift-upstream October 11, 2026 13:48 — with GitHub Actions Active
@oshaughnessy-junior
oshaughnessy-junior deployed to private-review-dispatch-rift-upstream October 11, 2026 13:57 — with GitHub Actions Active
- BasicIteration: join args_ile.txt lines with a space. Joining with ''
  then dropping the first space-separated token lost the first ILE option
  when the executable ended its own line, and a continuation backslash
  followed by spaces became a stray ' ' argument. Regression test runs the
  real statements on four layouts.
- test_calmarg_extra_args_transport: H_gwsignal asserted that lmax_nyquist
  alone switched calmarg to GWSignal, while ILE stays on the lalsuite route
  without --use-gwsignal. Split into H_lmax_lalsuite (no switch) and
  H_gwsignal (--use-gwsignal; dictionary nested as gwsignal_extra_kwargs).
- test_inert_keys_are_inert: a key named only in GWSignal's
  _RIFT_WRAPPER_CONTROLS is filtered, not read. Assert e_freq is in that set
  and absent from the rest of the file.
- test_rf_transverse_spin: read quote_arguments from argument_quoting.py,
  where this PR moved it (cip-startup KeyError).
- cip-startup: run the five new test files, failing on any skip.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@oshaughnessy-junior
oshaughnessy-junior deployed to private-review-dispatch-rift-upstream October 11, 2026 17:10 — with GitHub Actions Active
@oshaughnessy-junior

Copy link
Copy Markdown
Contributor Author

Adversarial review, 2026-10-11. Fixes pushed in 057dba9 (#438) and d7b5396 (#217).

Fixed

  • BasicIteration lost the first ILE option when args_ile.txt put the executable on its own line. Lines are now joined with a space; a new test covers four layouts and fails on the old line.
  • calmarg H_gwsignal asserted that lmax_nyquist alone switched calmarg to GWSignal, which ILE never does. It is now split into a lalsuite case and a --use-gwsignal case.
  • test_inert_keys_are_inert now accepts e_freq in _RIFT_WRAPPER_CONTROLS, and fails if it is removed from that set.
  • Preserve generic GWSignal options through production waveform paths #217: test_rf_transverse_spin now reads quote_arguments from argument_quoting.py.
  • The new test files are now gated: test-calmarg.sh (#438) and cip-startup (Preserve generic GWSignal options through production waveform paths #217), failing on any skip.

Checked on CIT (ldas-grid, IGWN python)

Check Result
Real pySEOBNR v5PHM (2,2) and (2,−2) now differ; identical on base
New tests on base code 185/194 fail
116 production args_ile.txt same argv as before, dict kept as one argument
--use-gwsignal production dicts 78/79 carry antisym flags; no RIFT control keys
#438 calmarg transport + alignment 28 passed, 1 known xfail
#217 new + calmarg + rf_transverse 310 passed, 1 known xfail

Behaviour change. A RIFT control key such as fd_centering_factor inside the GWSignal dict now raises TypeError from gwsignal; it was ignored before. No production config does this.

Not addressed. JAX ILE ignores these options (follow-up filed). Slurm calibration args and the other pipeline builders keep their pre-existing quoting.

🤖 Generated with Claude Code

@oshaughnessy-junior
oshaughnessy-junior marked this pull request as ready for review October 11, 2026 17:39
@oshaughnessy-junior
oshaughnessy-junior deployed to private-review-dispatch-rift-upstream October 11, 2026 17:39 — with GitHub Actions Active
…aries

- calibration_reweighting: restore parsing of the unquoted-key dictionary
  that existing O4c Calib_reweight.sub files deliver
  ("{antisymmetric_modes_hm: True, ...}"). The PR's second eval raised
  NameError there. Values are now parsed as literals ('True' -> True).
- pseudo_pipe: when the GWSignal dictionary carries lmax_nyquist, also emit
  --use-gwsignal-lmax-nyquist for calibration, as before. Calibration
  workers older than gwsignal_extra_kwargs read only that flag; without it
  their waveforms used a different lmax_nyquist from ILE's.
- rift.ini: 'gwsignal arguments' wins over 'arguments' again when both are
  given (base behaviour) instead of emitting both and failing.
- Regression tests for each; all fail on d7b5396.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…flag

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@oshaughnessy-junior
oshaughnessy-junior deployed to private-review-dispatch-rift-upstream October 11, 2026 23:55 — with GitHub Actions Active
@oshaughnessy-junior

Copy link
Copy Markdown
Contributor Author

Backward-compatibility review against the O4c production setup, 2026-10-11. Fixes in 33f8939 and 072daa8.

Production mixes versions: the asimov submit env runs RIFT 0.0.17.9 (8faa1c7), and workers run Reviewed-RIFT-20260807.sif (0.0.17.11). I re-rendered and rebuilt rift-v5PHM-calmarg-A (S241114bi) with real asimov for each version. I then ran the resulting ILE argv in the container, and the calibration argv through each worker's option code and waveform call.

Fixed

  • DAGs built by 0.0.17.9 pass calibration {antisymmetric_modes_hm: True, ...} with unquoted keys. This PR made every such job fail with NameError once the container is rebuilt. The legacy parser is restored.
  • With the current container, calibration lost lmax_nyquist while ILE kept 1. The --use-gwsignal-lmax-nyquist flag is emitted again.
  • gwsignal arguments again wins over arguments in rift.ini.

With the fixes, every submit/worker mix runs. The antisymmetric options reach pySEOBNR whenever the worker includes this PR.

Record and scripts on CIT: ~/LVK/IR1/code_validation/o4c-gwsignal-options-pr217-20261011/. O4d port: oshaughnessy-junior#442.

🤖 Generated with Claude Code

@oshaughn
oshaughn merged commit 91e427a into oshaughn:rift_O4c Oct 12, 2026
5 checks passed
@oshaughn
oshaughn deployed to private-review-dispatch-rift-upstream October 12, 2026 10:54 — with GitHub Actions Active

This branch was successfully deployed

1 active deployment
private-review-dispatch-rift-upstream — 072daa86 Deployed Oct 12, 2026 by oshaughn via Dispatch exact upstream RIFT PR generation #198
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants