Conversation
EcoswEsinwRV().default_prior(sigma_K0=...) gives rv_semiamp a PeriodDependentKPrior, whose __call__ reads params["eccentricity"]. That parameterization carries (ecosw, esinw) instead, so the key does not exist and every sampler run raised KeyError -- the documented happy path was unusable. The parameterization already knows the conversion, so rather than duplicating sqrt(ecosw^2 + esinw^2) inside the prior, AbstractParameterization grows a derived_eccentricity() hook returning None by default. That default covers both parameterizations carrying eccentricity outright (nothing to derive) and the Kepler-free Fourier bases (none exists); EcoswEsinwRV overrides it by delegating to its existing eccentricity(). The values dict handed to callable priors is enriched at the point of use, so any future eccentricity-dependent prior works too. Reading .parameterization off the component model at those seven sites also needs it declared on AbstractComponentModel, which previously declared only extensions. Declaring it as an eqx.AbstractVar matches how extensions is already declared and makes the class docstring true: it has always said component models "carry only parameterization and extensions", while only one of the two was actually part of the interface. Shared priors in a JointModel are left alone on purpose: components need not agree on a parameterization, so there is no unambiguous value to derive. The three existing EcoswEsinwRV prior tests only *constructed* the prior and inspected its keys, which is why this survived. The new tests evaluate it through an actual run, and fail with the original KeyError when the fix is reverted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fixing the KeyError exposed the next problem on the same path. default_prior puts independent Uniform(-1, 1) priors on ecosw and esinw, so the support is a square while a bound orbit needs the unit disk: ~21% of draws (1 - pi/4) have e = sqrt(ecosw^2 + esinw^2) >= 1, where the K prior's (1 - e^2)^(-1/2) is NaN. NaN does not behave like a rejected sample. It propagates through the max reduction the rejection step normalizes by, so max_log_likelihood, logZ_int and logZ_int_ess all come back NaN and nothing is accepted -- silently. Measured on 4000 draws: max_log_likelihood=nan without ignore_non_finite, finite with it. ignore_non_finite=True is the existing, correct remedy, so this documents the trap in sharp-bits.md and the spec rather than changing the prior -- narrowing the support is a modelling decision, not a bugfix, and is tracked separately. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Its default prior's support is a square, so about 21% of draws are unbound orbits (e >= 1). ignore_non_finite=True already stops those from silently NaN-poisoning the evidence statistics, but the fifth of every prior library they waste is real and unfixed. The fix is not local. harv.stats.numpyro_ext.UnitDisk is the right distribution but is two-dimensional (event_shape=(2,)), while HarvPrior.nonlinear_priors holds one scalar prior per name. Supporting it means adding a joint multi-name nonlinear prior to the public API, which the spec has to define before any implementation, and it touches seven load-bearing sites. Recorded as a new "Known bugs" section rather than a separate BUGS.md, so that deferred defects sit beside the design they contradict and under the same rule that governs the rest of the spec. The section states the distinction from "Planned features and known gaps": bugs are behavior that is wrong, gaps are behavior that is absent. Includes the measurements, the seven sites, and the three alternatives already ruled out, so picking this up later does not mean re-deriving the analysis. 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
1 participant
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.
Fixes a bug where the
EcoswEsinwRVis actually unusable with its owndefault_priorin rejection sampling because it doesn't have aneccentricityparameter. This also identified a bug/limitation of the current design in that we can't place a joint prior over two parameters (as would be needed for ecosw/esinw to come fromUnitDisk) - the bug is reported in docs/spec.md under "known bugs".