CIP: fix the log-uniform eccentricity prior (np.ln, wrong normalization), and give the CIP priors a test - #174
Open
oshaughnessy-junior wants to merge 1 commit into
Conversation
oshaughnessy-junior
had a problem deploying
to
private-review-dispatch-rift-upstream
August 13, 2026 17:20 — with
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.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
oshaughnessy-junior
force-pushed
the
rift_O4c_ecc_prior_fix
branch
from
August 13, 2026 17:20
5a3c7a1 to
e1f49ae
Compare
oshaughnessy-junior
had a problem deploying
to
private-review-dispatch-rift-upstream
August 13, 2026 17:21 — with
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
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.
--eccentricity-prior log_uniform, added in 0.0.17.12 (rapidpe-rift/rift!54), could never have run.log_eccentricity_priorcallsnp.ln, which does not exist. The option raises as soon as CIP evaluates the prior.Independently of the crash, the normalization was the uniform prior's. For a density uniform in ln(e) on
[ECC_MIN, ECC_MAX]:not
ln(ECC_MAX - ECC_MIN). With the shipped ecc-min auto-correction of 0.001 and ecc-max 0.4, the old constant isln(0.399) = -0.918— negative, so the prior would have been negative everywhere had it evaluated at all. The adjacentuniform_eccentricity_ln_prioralready had the correctln(ECC_MAX/ECC_MIN)form, which is what made the discrepancy visible.The fix is one line plus a comment recording the derivation.
Why it shipped, and the test that closes the gap
CIP is a script, not an importable module — importing it parses argv, reads the input grid and runs several thousand lines of module-level setup. So none of its 38 prior densities has ever had a test, and a function that cannot execute at all reached a release.
test_cip_priors.pycloses that. It deliberately does not transcribe the priors: a copied reference implementation drifts from the shipped code and then tests nothing. Instead it extracts the realdefblocks from the CIP source withastand 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, and no argparse or I/O executes.Two layers, covering multiple priors rather than just the one that broke:
test_prior_evaluatesnp.ln), non-finite or negative densities, non-broadcastable returns — for any prior, including ones added later, with no new test to writetest_prior_is_normalizedThe integration measure is spelled out per prior (
dx,d(ln x),d(x²)) rather than inferred, since choosing it wrongly is precisely the bug class under test. Priors documented in-source as unnormalized (M_prior,q_prior,unnormalized_*, the tapered magnitudes, …) are listed inUNNORMALIZEDand excluded on purpose, not by oversight.Plus two shape tests specific to this regression: that
e·p(e)is constant (sopreally 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-priorwould silently stop doing anything.There is also a
test_priors_were_actually_extractedguard: if CIP is refactored so the priors are no longer top-level, every parametrized test would collect zero cases and the suite would go green while testing nothing. It fails loudly instead.Verified as a regression test
Against the unfixed 0.0.17.12 CIP: 4 failed, 56 passed — all four naming
log_eccentricity_prior(test_prior_evaluates,test_prior_is_normalized, and both shape tests). Against the fix: 60 passed. Runs in under a second and needs only numpy + scipy.Two things found while writing it, and what I did about them
Selection is on
'prior'appearing anywhere in the name, not a_priorsuffix. The obvious suffix rule silently skipss_component_zprior,s_component_zprior_positiveand both*volumetricpriordensities — most of the spin sector. Worth knowing if anyone extends this.The test does not import
lalsimutils(it parsesp_Rout of the source instead). Importing it pulls in LAL, whose default error handler callsabort(), which turns any numerical complaint raised insidescipy.integrate.quadinto a process core dump instead of a test failure. I hit exactly that while developing this. These priors are pure numpy, so the test stays free of that stack.Not changed here, flagged for a maintainer:
s_component_volumetricpriorintegrates toR/9over[0,R], not 1. It is excluded from the normalization test as an unnormalized density. I have not verified whether that factor is intended (a volumetric spin-magnitude prior is usually3a²), so I have not touched it — flagging only.util_ConstructIntrinsicPosterior_GenericCoordinates.py:1007readsprior_map["s1z_bar"] = s_compone+nt_zprior_positive— a typo that will raiseNameErrorwhenever--aligned-prior alignedspin-zprior-positiveis used. Out of scope for this PR; happy to fix separately.This same fix and test are also carried on the O4d consolidation branch (#173 / oshaughnessy-junior#91), so the two lines converge rather than diverge on this function.
🤖 Generated with Claude Code