Bugs and inconsistencies found during the architecture review#

Found while reading the api_generalisation branch (head b8bc7d8) for the ground-up design comparison in groundup_design.md. Every item was verified by reading the code at the cited lines (line numbers refer to b8bc7d8). Items are grouped by how sure I am that they are wrong rather than merely fragile.

Status: every item except 11 is fixed on this branch; each carries a Resolution line. Item 11 is a design-level change and is deferred to groundup_design.md.

Confirmed bugs#

1. IsobaricAnalogPNObservation silently drops two solver arguments#

  • Where: src/rxmc/ias_pn_observation.py:52-53 (constructor signature) and :113-120 (the set_up_solver call).

  • What: the constructor accepts wavelengths_beyond_range and zeros_per_node, documents them (:92-94), and set_up_solver takes them (:209-210), but the call inside __init__ never forwards them. The defaults are always used. The elastic observation forwards both (src/rxmc/elastic_diffxs_observation.py:149-150).

  • Why it matters: a user tuning the Lagrange basis size for the (p,n) IAS channel gets no effect and no error.

  • Fix: add the two keyword arguments to the set_up_solver call.

  • Resolution: forwarded; test_reaction_observation.py::TestSolverSettingsForwarding asserts the kwargs reach set_up_solver for both reaction observations.

2. BatchedAdaptiveMetropolisSampler adapts its proposal during burn-in#

  • Where: src/rxmc/param_sampling.py:376-388.

  • What: the class docstring (:304), the sample docstring (:354) and the burn parameter docstring (:366-368) all say the proposal covariance is replaced only after non-burn batches. The code recomputes self.proposal_cov and self.args unconditionally; only record_batch is guarded by if not burn.

  • Why it matters: burn-in batches feed the adaptation, so the behaviour differs from what the docstring promises and from AdaptiveMetropolisSampler. Either the doc or the code is wrong. Adapting during burn-in is arguably the better behaviour, so the likely fix is to the docstrings.

  • Fix: decide, then make the docstrings and the if not burn: guard agree.

  • Resolution: kept the code behaviour (adapt after every batch, burn-in included) and fixed the three docstrings. sample now also refreshes self.proposal so it never goes stale; pinned by test_sampler.py::TestSamplerPriors::test_batched_adaptive_updates_proposal_after_burn_batch.

3. Parameter defines __eq__ without __hash__#

  • Where: src/rxmc/params.py:39-48.

  • What: defining __eq__ sets __hash__ = None, so Parameter objects are unhashable. Every uniqueness check in the package keys on id(p) or p.name for this reason (covariance.py, constraint.py, evidence.py), and nothing documents it.

  • Why it matters: set(model.params) or {p: value} raises TypeError at runtime. It is a trap for anyone extending the package, and it is the root reason the by-identity routing needs id() bookkeeping.

  • Fix: either drop __eq__ (identity is the sharing semantics anyway) or add __hash__ = object.__hash__. See groundup_design.md §2.1.

  • Resolution: value-based __hash__ consistent with the existing value __eq__; bounds is coerced to a 2-tuple of floats so the hash is stable; __repr__ added. Identity routing in covariance.py / evidence.py is untouched. New test/test_params.py.

4. Two independent pint UnitRegistry instances#

  • Where: src/rxmc/elastic_diffxs_observation.py:24 and src/rxmc/ias_pn_observation.py:12.

  • What: each module builds its own registry. pint refuses to combine quantities from different registries.

  • Why it matters: latent today because no code path mixes the two, but any helper that takes a quantity from one module into the other will raise. DEFAULT_LMAX = 20 is likewise duplicated (:27 and :14).

  • Fix: one ureg in a shared module (observation_from_measurement.py is the natural home; its docstring already says it holds what the two share).

  • Resolution: ureg, DEFAULT_LMAX, XS_UNIT, RUTHERFORD_UNIT and MB_PER_B live in observation_from_measurement.py (now exported from rxmc); both observation modules import and re-export them. test_reaction_observation.py::TestSharedUnits.

Inconsistencies between the two sampler front ends#

CalibrationConfig and Walker are meant to be two drivers over the same posterior. They are not.

5. Walker.log_posterior evaluates the likelihood when the prior is -inf#

  • Where: src/rxmc/walker.py:140-143 versus src/rxmc/config.py:405-411.

  • What: the config path short-circuits on a non-finite prior and skips the forward model. The walker path always calls Evidence.log_likelihood first. The Gibbs conditional has the same split: config.conditional_posterior (config.py:579-583) short-circuits, the inline closure at walker.py:130-132 does not.

  • Why it matters: out-of-bounds proposals cost a full reaction-model solve in the walker. With bounds also enforced inside the kernels this is a performance bug, not a correctness bug, but it is a silent divergence.

  • Resolution: Walker.log_posterior and the Gibbs closure evaluate the prior first and return -inf without touching the likelihood. test_sampler.py::TestWalkerPosterior::test_*_skips_likelihood_when_prior_neg_inf.

6. Tempering exists only on the config path#

  • Where: config.py:245-252, 384, 583; no counterpart in walker.py.

  • What: likelihood_scaling scales the likelihood (and the Gibbs conditionals) in CalibrationConfig. Walker has no such knob; the only way to temper is Evidence(weights=...). examples/overconfidence.ipynb demonstrates both and prints a check that they agree.

  • Why it matters: two names for one concept, with one of them reachable from only one driver.

  • Resolution: Walker(..., likelihood_scaling=) added with the same semantics as the config (scales the likelihood in the model block and the Gibbs conditionals, never the prior). test_sampler.py::TestWalkerPosterior::test_*_applies_likelihood_scaling*.

7. List-of-scipy priors are accepted by one driver and rejected by the other#

  • Where: config.py:133-135, 153-155, 184-185 (list branches in ParameterConfig) versus param_sampling.py:66-70 (_validate_object requires prior.logpdf) and walker.py:131, 160 (calls sampler.prior.logpdf directly).

  • What: ParameterConfig special-cases a plain list of frozen scipy distributions in three places. A Sampler built with the same list fails at construction because a list has no logpdf.

  • Why it matters: the prior protocol is documented as one thing and implemented as two. IndependentPrior already exists to wrap a list; ParameterConfig could wrap in __init__ and delete all three branches.

  • Resolution: rxmc.priors.as_prior wraps a list/tuple in IndependentPrior; both ParameterConfig.__init__ and Sampler.__init__ call it, and the four list branches in ParameterConfig are gone. Behaviour change: x0 for a list prior is now seeded (IndependentPrior default seed) instead of drawing from numpy’s global state, and config.prior returns the wrapper. Tests in test_config.py, test_sampler.py, test_priors.py.

8. ParameterConfig._infer_dim misreads priors whose mean is a method#

  • Where: src/rxmc/config.py:92-98.

  • What: if the prior has no integer dim, the fallback is int(np.size(dist.mean)). For any object whose mean is a method this is 1, regardless of the true dimension.

  • Why it matters: a custom multi-dimensional prior exposing mean() is reported as one-dimensional and rejected at config.py:109-113 with a misleading message. Frozen scipy multivariate distributions happen to expose mean as an array, which is why the tests pass.

  • Fix: call mean if callable, or require dim and drop the guess.

  • Resolution: an integer dim wins; otherwise mean is called when it is a method. test_config.py::test_infer_dim_calls_mean_method.

Fragile, not wrong#

These are not bugs today but each is one refactor away from becoming one.

9. Unit conventions split across model and observation with no shared constant#

  • elastic_diffxs_model.py:166 and ias_pn_model.py:124, 170 divide by a bare 1000 (mb/sr to b/sr). The matching assumption lives in the observation as ureg.millibarn / ureg.steradian (elastic_diffxs_observation.py:204). Nothing ties them together.

  • Resolution: both models divide by MB_PER_B, derived from the shared registry next to XS_UNIT / RUTHERFORD_UNIT, which the observations now use. test_reaction_observation.py::TestSharedUnits::test_unit_constants_agree.

10. Model and observation compatibility is checked by string, or not at all#

  • elastic_diffxs_model.py:137, 174 compare observation.quantity to self.quantity. ias_pn_model.py:135, 159 reach straight for observation.constraint_workspace with no check. Pairing an elastic model with an IAS observation fails inside jitr with a shape error.

  • Resolution: each model checks isinstance against its observation class first in evaluate and visualizable_model_prediction and raises a named ValueError (a string check cannot work: both observations report quantity == "dXS/dA"). test_reaction_models.py::TestObservationTypeChecks.

11. Masked views share solver workspaces by reference, routed by id()#

  • observation.py:204 uses copy.copy, so a masked view of a reaction observation shares both jitr workspaces and every array with its root. transforms.py:281, 301 route per_observation_scaling by id(obs.identity), and test_holds_observation_references exists only to stop id recycling. Any deep copy, pickle, or reconstruction of an observation breaks the routing with a KeyError at evaluation time.

  • Deferred: design-level; see groundup_design.md §2.3–2.4 (bind-time predictors, blocks without identity keys).

12. The burn-in loop in Walker.walk duplicates the main loop#

  • walker.py:207-221 versus :229-241: identical bodies apart from burn=True and the progress string. Any change to one must be mirrored.

  • Resolution: one _run_batch(steps, burn) sweep plus _batch_message; the burn-in line prints no acceptance fraction because nothing is recorded during burn-in. test_sampler.py::TestWalkerPosterior::test_burn_message_has_no_acceptance_fraction.

13. prior_transform clips the unit cube only at the top level#

  • config.py:492-506 clips u to [eps, 1-eps]; ParameterConfig.prior_transform and IndependentPrior.prior_transform (priors.py:273-277) do not. Calling either directly with an exact 0.0 or 1.0 returns ±inf. (TruncatedNormalPrior is finite at the boundary by construction.)

  • Resolution: rxmc.priors.clip_unit_cube is applied in all four transforms. test_priors.py::TestUnitCubeClipping, test_config.py::test_prior_transform_boundary_finite.

Already fixed on this branch#

  • The _rows row-count check in model_comparison.py was reported by an earlier read as unable to fire. At b8bc7d8 it is called without n for the model samples and with n for the covariance samples (:138, 143), which is correct.