orbit: the V_c/V_e eccentricity parameterization (fitvcve) - #186
Merged
Conversation
`fitvcve: true` now works, per orbit. It samples V_c/V_e -- the ratio of the
speed a planet would have on a circular orbit of the same period to its speed at
transit -- and the direction of omega, instead of sqrt(e)cos(omega) /
sqrt(e)sin(omega). The reason is Eastman (2024, PASP; arXiv:2309.14410): over
330 simulated systems, a transit-only fit in the sqrt(e) pair recovers
eccentricities that are measurably WRONG, while the transit duration constrains
V_c/V_e = sqrt(1 - e^2)/(1 + e sin omega) tightly.
The stubs this replaces had been declared and unbuilt since before the manifest
migration; `fitvcve: true` raised. Review 8.2.1 removed the structural half of
the blocker (per-element roles), and this is the rest.
WHAT THE PAPER DOES NOT ADDRESS, AND THIS DOES
The paper picks one root of the quadratic inversion with a discrete sign
parameter S, and rejects combinations with no real root. Both are fine for
DE-MC and useless to a gradient sampler: S is piecewise constant (no gradient,
and the logp jumps at S = 1), and a rejection is a wall with nothing to follow
back out. So:
* the likelihood is MARGINALIZED over both roots (System._add_branch_mixtures,
one logsumexp over the 2^k root combinations of k V_c/V_e orbits, logged, with
a warning past two). Exact to 1e-12 against a closed-form two-branch
marginalization -- and against a FOUR-branch one, which is a separate claim:
two orbits are two elements of one `ecc` vector, so both branches replace the
same node, and merging their substitutions into one dict (the obvious
implementation, and the first one here) kept the last and covered 3 of the 4.
They are applied in sequence instead, which composes because each replacement
reads the node it replaces -- checked at declaration, since the failure is
silent and the test that catches it needs branch offsets small enough that no
single term dominates the logsumexp;
* the discriminant is floored INSIDE the sqrt (hard: a NaN is unbuildable) while
the UNFLOORED value drives a soft_lower_bound (soft: the floored one is flat
across the whole imaginary region, and a flat penalty has no gradient) -- the
same split `ecc_from_sqrte` makes for the collision bound.
The mixture needs no data component to know about it: it substitutes the ecc
node into the RV-LEVEL term sum and adds `logsumexp(...) - L_ref`, which cancels
the copy PyMC already added. Two consequences, both wanted -- a term that does
not depend on ecc factors out identically, and one that does (the Jacobian, the
collision bound, and a future orbit-crossing penalty coupling two orbits,
review 8.8.9) is replicated per branch, making each branch's weight
`log w + log|J| + barriers`: the form review 8.4.4 specifies for folded
likelihoods, for free.
THE PRIOR, AND THE SIGN OF IT
V_c/V_e is the sampled coordinate, so the eccentricity it derives inherits
p(e) ~ |d(V_c/V_e)/de|, which diverges as e -> 1: the paper's "strongly biases e
toward high eccentricities" (section 3). Flattening it means adding the
RECIPROCAL, so the likelihood carries MINUS log|d(V_c/V_e)/de|. Adding the
derivative -- what the first draft did, on a misremembered reading of
EXOFASTv2's determinant -- doubles the bias instead of removing it, and no check
of the derivative's MAGNITUDE can see the difference: the finite-difference test
passes either way. So the direction is pinned by measuring the implied density
on e for flatness, and the potential's own sign is pinned against that at the
call site. EXOFASTv2's convention is deliberately not restated anywhere in the
comments, because it could not be checked from this tree.
Built from the ecc/omega NODES so the mixture makes it per-branch, which is
right: the two roots have different Jacobians. It declares itself to the
reporting layer, so the tables say "uniform in e (Jacobian applied)" rather than
"Uniform" on V_c/V_e, and it carries a prose sentence citing the paper (bib
entry included).
FIVE BUGS THE VERIFICATION CAUGHT, ALL MINE
1. A dropped factor of 2 in the discriminant: `B^2 - 4AC = 4(1 - x^2 cos^2 w)`,
so the roots need `(-B +/- 2 sqrt(...))/2A`. The first draft's roots were
real, in range, and wrong. Caught by round-tripping the inversion against
eq 4 over an (e, omega) grid -- which is now a test, for that reason.
2. "Primary = the lower root" is wrong. The roots' product is C/A, so for
V_c/V_e < 1 they have opposite signs and the physical one is always the
upper; preferring the lower would have pinned the forward model at e = 0,
with no gradient, across that whole region. (EXOFASTv2 does prefer the
lower -- but only among roots it has already tested for being physical.)
3. `orbit.tp` CONSUMES secosw/sesinw, which a V_c/V_e orbit reports rather than
samples -- so it would have read their pre-patch placeholder: a silently
wrong periastron. Fixed with per-orbit `(e, omega)` expressions for `tp`,
`esinw` and `ecosw`, and `calc_tp_from_ecc` is strictly better behaved
besides (writing the true anomaly at conjunction as f = pi/2 - omega cancels
the sqrt(e) factor, removing both of `calc_tp`'s e = 0 singularities --
review 1.8.2, which the mixture had made reachable systematically since an
unphysical branch clips to exactly zero). It agrees with `calc_tp` up to a
whole period, which every consumer is invariant to.
4. The Jacobian's sign, above.
5. `vcve`'s upper bound of 2.0 looked generous and caps e at 0.6 for an orbit
transiting near APASTRON (eq 4 at omega = -90 deg is
sqrt((1+e)/(1-e))) -- silently excluding exactly the eccentric transiting
planets the parameterization exists to measure. It is now the image of the
eccentricity cap, sqrt((1+MAX_ECC)/(1-MAX_ECC)) = 141.4. Nothing restates
the paper's collision limit 2*sqrt(a/R* + R_P/R* - 1): this component already
enforces it as a SOFT bound on e itself, and a hard bound on another
coordinate would be a second, disagreeing spelling of one limit.
Bug 3 is why `System._validate_reported_not_consumed` exists (stage 2b): the
"nothing consumes a reported element" property is what the whole two-phase build
rests on, and breaking it is silent. It checks per element, so a legal mixed
system still works, and it found `esinw`/`ecosw` the moment it caught `tp`.
ALSO HERE
* Role 3's deferred build pass -- the last piece of 8.2.1, which 8.8.3 owned:
`Component.add_parameter` defers the whole wiring (not just the patch, since
resolving a reported expression's deps mid-build would recurse),
`finalize_reported` -> `finalize_deferred` applies it after stage 6.
* `vcve` carries `rank: 5`, exactly as `planet.log_q` does, so the relaxation
engine rewrites IT rather than pushing back on ecc/omega. Without it the
parameter's mere DEFAULT competed with the sqrt(e) defaults and the engine
rewrote ecc/omega to satisfy it, moving the start of four shipped examples and
(through K) their planet masses. The symbolic bridge is written forward so
one params file drives either parameterization -- seeding ecc/omega gives a
V_c/V_e start, and vice versa.
* `parameterization.merge_options`, and `mode_manifest` learns
`output_expr_key`. A manifest entry may be None or a bare string, so the
`{**entry, ...}` splat in `_restrict_bigomega_halfplane` broke the moment a
free parameter was spelled None -- review 4.5.3's hazard in a third place.
* The start avoids the parameterization's own degeneracy: V_c/V_e = 1 at
omega = 0 IS the circular orbit and is exactly where the two roots merge and
de/d(V_c/V_e) is infinite. Defaults start at 0.99 with omega = 90 deg, where
the discriminant is 1 -- the same reasoning that starts secosw/sesinw at 0.01
rather than 0.
STILL GUARDED: the chord half (`fitchord: true`, `cosi`'s `from_b`, the
orbit-level `b`), whose physics is still undefined. A `fitchord` that merely
FOLLOWS `fitvcve` does not raise -- it was not asked for. The transit-only
DEFAULT (both halves on for an orbit constrained by transits alone) flips when
that lands, so the pair the paper validated turns on together and the shipped
transit-only examples move exactly once.
VERIFICATION: all 19 shipped example configs keep their exact start logp,
free-RV set and sampled structure -- the only difference is the new `orbit.vcve`
row, now REPORTED on every orbit, whose values sanity-check the forward relation
(hd80606's e ~ 0.93 gives 1.82, hat3's near-circular orbit 0.99986, GaiaBH1's
e ~ 0.45 gives 0.81). Full suite green. A fitvcve transit fit really samples
under nuts_sampler="numpyro" -- three separate places in this feature could
produce a NaN only in a JAX backward pass. Tests: tests/test_vcve.py (72),
plus the rewritten fitvcve guards in tests/test_wip_guards.py.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The first CI run of the V_c/V_e work did not fail on a bug: three Ubuntu jobs were CANCELLED at exactly 45 minutes -- `timeout-minutes`, not fail-fast, which that matrix disables -- and the macOS job spent 40 minutes and then died in the pytensor compile lock, which is the same "everything is slower" symptom wearing a different hat. Master's own jobs take ~34 minutes, so the budget was ~11 minutes and this file, cold, wanted 18.5 of them. `--dist loadfile` pins a file to ONE worker, so none of that parallelizes away. Three cuts, none of which drops a claim: * The two-orbit tests ran on a TRANSIT model. The mixture replicates the whole likelihood per root combination, so the both-vcve model was four copies of a transit likelihood -- to make a statement about element ROLES and BRANCH COUNT that no part of the transit forward model participates in. They use a small RV dataset now. * The both-vcve case is built and not evaluated. What it adds over the mixed case is that two branches naming the same node coexist, which is a property of the DECLARATION; the arithmetic of combining them is measured exactly, and in milliseconds, by the toy four-branch test. * The single-orbit transit model is built ONCE (a module-scoped fixture; three tests asked different questions of the same model and rebuilt it each time), and its dlogp is no longer compiled -- cold, that one compile was the most expensive thing in the file, and both backends' gradients through exactly these graphs are still checked: the JAX one by the numpyro leg, which samples that very model, and the C one by the two-orbit mixed test, which is the same mixture over a cheaper likelihood. Measured with a fresh compiledir (`PYTENSOR_FLAGS=base_compiledir=...`), which is what CI pays: the workflow caches the Zenodo spectra and the astropy kernel and deliberately caches no compiledir. 18:28 -> 4:11 cold, 4:44 -> 1:58 warm, same 79 tests. Worth knowing for the next test-adding PR: the 45-minute cap is ~11 minutes away from master on a good day, and the workflow's own comment still claims the suite takes "12 to 16 minutes", which it has not for some time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
fitvcve: truenow works, per orbit: it samples V_c/V_e -- the ratio of thespeed a planet would have on a circular orbit of the same period to its speed at
transit -- and the direction of omega, instead of sqrt(e)cos(omega) /
sqrt(e)sin(omega). The reason is Eastman (2024, PASP; arXiv:2309.14410): over 330
simulated systems, a transit-only fit in the sqrt(e) pair recovers eccentricities
that are measurably wrong, while the transit duration constrains
V_c/V_e = sqrt(1 - e^2)/(1 + e sin omega)tightly. Review 8.8.3; the stubs itreplaces raised.
What the paper does not address, and this does
The paper picks a root of the quadratic inversion with a discrete sign parameter
S, and rejects combinations with no real root. Both are fine for DE-MC anduseless to a gradient sampler --
Sis piecewise constant with a logp that jumpsat
S = 1, and a rejection is a wall with nothing to follow back out. So:System._add_branch_mixtures:one
logsumexpover the2^kroot combinations ofkV_c/V_e orbits, logged,warning past two). It substitutes into the RV-level term sum and adds
logsumexp(...) - L_ref, which cancels the copy PyMC already added -- so nodata component knows this is happening, a term independent of
eccfactors outidentically, and one that depends on it (the Jacobian, the collision bound, a
future orbit-crossing penalty) is replicated per branch, making each branch's
weight
log w + log|J| + barriers: the form review 8.4.4 specifies for foldedlikelihoods, for free;
while the unfloored value drives a
soft_lower_bound(soft: the floored oneis flat across the whole imaginary region, and a flat penalty has no gradient).
Also here: role 3's deferred build pass (the last piece of 8.2.1, which 8.8.3
owned),
vcveatrank: 5so the relaxation engine rewrites it rather thanpushing back on
ecc/omega, andparameterization.merge_options.Five bugs the verification caught, all mine
B^2 - 4AC = 4(1 - x^2 cos^2 w),so the roots need
(-B +/- 2 sqrt(...))/2A. The first draft's roots were real,in range, and wrong -- caught by round-tripping the inversion against eq 4 over
an (e, omega) grid, which is now a test for exactly that reason.
C/A, so belowV_c/V_e = 1they have opposite signs and the physical one is always theupper. Preferring the lower would have pinned the forward model at
e = 0,with no gradient, across that whole region.
orbit.tpconsumed a reported element. It readssecosw/sesinw, which aV_c/V_e orbit reports rather than samples, so it would have read the
pre-patch placeholder: a silently wrong periastron. Fixed with per-orbit
(e, omega)expressions fortp,esinwandecosw-- andcalc_tp_from_eccis strictly better behaved besides (writing the trueanomaly at conjunction as
f = pi/2 - omegacancels thesqrt(e)factor,removing both of
calc_tp'se = 0singularities, review 1.8.2). This is whySystem._validate_reported_not_consumednow exists; it foundesinw/ecoswthe moment it caught
tp.p(e) ~ |d(V_c/V_e)/de|-- the paper's high-eccentricity bias -- andflattening it means adding the reciprocal. Adding the derivative doubles
the bias, and the finite-difference test passes either way, so the direction is
now pinned by measuring the implied density on
efor flatness plus acall-site test against that. (EXOFASTv2's convention is deliberately not
restated in the comments: it could not be checked from this tree.)
vcve's upper bound of 2.0 cappedeat 0.6 for an orbit transiting nearapastron (eq 4 at
omega = -90 degissqrt((1+e)/(1-e))) -- silentlyexcluding exactly the eccentric transiting planets the parameterization exists
to measure. It is now the image of the eccentricity cap,
sqrt((1+MAX_ECC)/(1-MAX_ECC)) = 141.4.A sixth was in the mixture itself: with two V_c/V_e orbits, both branches replace
the same
eccnode, and merging their replacements into one dict kept only thelast -- silently marginalizing over 3 of the 4 combinations. They are applied in
sequence now (which composes, because each replacement reads the node it
replaces), checked at declaration, and measured against a closed-form four-branch
marginalization whose branch offsets are deliberately small: with a far-away
branch the logsumexp is dominated by one term and the missing combination is
invisible.
Still guarded
The chord half (
fitchord: true,cosi'sfrom_b, the orbit-levelb),whose physics is undefined. A
fitchordthat merely followsfitvcvedoes notraise -- it was not asked for. The transit-only default (both halves on for an
orbit constrained by transits alone) flips when that lands, so the pair the paper
validated turns on together and the shipped transit-only examples move exactly
once.
Verification
sampled structure. The only difference is the new
orbit.vcverow, nowREPORTED on every orbit, and its values sanity-check the forward relation
(hd80606's
e ~ 0.93gives 1.82, hat3's near-circular orbit 0.99986,GaiaBH1's
e ~ 0.45gives 0.81).tests/test_vcve.pyis 79 tests; the fitvcveguards in
tests/test_wip_guards.pyare rewritten for the new behavior.nuts_sampler="numpyro"-- threeseparate places in this feature could produce a NaN only in a JAX backward
pass.
🤖 Generated with Claude Code