Skip to content

Fix silent likelihood/reporting corruption in the Dynesty log-likelihood pipeline - #88

Open
AstroEloy wants to merge 1 commit into
wmpg:masterfrom
AstroEloy:errors-dynesty
Open

Fix silent likelihood/reporting corruption in the Dynesty log-likelihood pipeline#88
AstroEloy wants to merge 1 commit into
wmpg:masterfrom
AstroEloy:errors-dynesty

Conversation

@AstroEloy

Copy link
Copy Markdown
Contributor

Four bugs in DynestyMetSim.py let a bad model draw leak a finite, misleading
value into the nested-sampling likelihood surface (or crash the results
summary) instead of being rejected/surfaced correctly:

  1. np.interp() in logLikelihoodDynesty (simulated_time, simulated_lc_intensity,
    simulated_lag) clamped out-of-range height queries to the simulation's edge
    value instead of returning NaN. A simulation that terminates before
    covering the full observed height range (e.g. the meteoroid fully ablates
    or hits h_kill/v_kill early) therefore slipped past the NaN-count guard
    that was supposed to reject it, and got scored against a duplicated final
    value instead of being rejected. Fixed by passing left=np.nan, right=np.nan,
    matching the pattern already used correctly elsewhere in the file
    (_worker_simulate_and_interp).

  2. The frame-integration branch of the luminosity comparison (the common
    case, since camera frame time is normally longer than the simulation step)
    never checked that integrateLuminosity()'s output covered the same points
    as the plain-interpolation branch did. Added the missing NaN-count guard
    to that branch.

  3. runSimulationDynesty caught ZeroDivisionError from the simulation and
    silently retried with a brand new, unrelated nominal Constants() object,
    discarding the sampled parameters entirely. dynesty would then score that
    live point using a completely different model without knowing it. Fixed by
    letting the exception propagate and having logLikelihoodDynesty reject the
    point with -inf, the same way it already handles a timeout.

  4. summaryResultsTable's marginal-mode histogram (Mode_{Ndim} column) ran on
    the raw, NaN-containing samples/weights instead of the already-masked
    x_valid/w_valid used for every other statistic in the same loop. np.min/
    np.max of a NaN-containing array is NaN, and np.histogram raises ValueError
    on a NaN range, so this crashed the entire results/plotting stage whenever
    any parameter had a NaN sample. The same histogram-plus-mode logic was
    duplicated (correctly, with masking) in the posterior distribution plot
    code a few hundred lines down, which is how the divergence was caught.
    Extracted a shared _weightedHistogramMode() helper used by both call
    sites, so there's now a single place responsible for this computation.

Test plan:

  • Added wmpl/Dynesty/Tests/test_DynestyMetSim.py: 6 regression tests covering
    all four bugs. runSimulationDynesty/runSimulation/constructConstants are
    mocked out with hand-built SimpleNamespace stand-ins, so the suite runs in
    milliseconds with no real MetSim/dynesty evaluation.
  • Verified each test fails against the pre-fix code (via git stash) and
    passes with the fixes applied.
  • Run with: python -m wmpl.Dynesty.Tests.test_DynestyMetSim

🤖 Co-Authored-By: Claude Opus 4.8

…ood pipeline

Four bugs in DynestyMetSim.py let a bad model draw leak a finite, misleading
value into the nested-sampling likelihood surface (or crash the results
summary) instead of being rejected/surfaced correctly:

1. np.interp() in logLikelihoodDynesty (simulated_time, simulated_lc_intensity,
   simulated_lag) clamped out-of-range height queries to the simulation's edge
   value instead of returning NaN. A simulation that terminates before
   covering the full observed height range (e.g. the meteoroid fully ablates
   or hits h_kill/v_kill early) therefore slipped past the NaN-count guard
   that was supposed to reject it, and got scored against a duplicated final
   value instead of being rejected. Fixed by passing left=np.nan, right=np.nan,
   matching the pattern already used correctly elsewhere in the file
   (_worker_simulate_and_interp).

2. The frame-integration branch of the luminosity comparison (the common
   case, since camera frame time is normally longer than the simulation step)
   never checked that integrateLuminosity()'s output covered the same points
   as the plain-interpolation branch did. Added the missing NaN-count guard
   to that branch.

3. runSimulationDynesty caught ZeroDivisionError from the simulation and
   silently retried with a brand new, unrelated nominal Constants() object,
   discarding the sampled parameters entirely. dynesty would then score that
   live point using a completely different model without knowing it. Fixed by
   letting the exception propagate and having logLikelihoodDynesty reject the
   point with -inf, the same way it already handles a timeout.

4. summaryResultsTable's marginal-mode histogram (Mode_{Ndim} column) ran on
   the raw, NaN-containing samples/weights instead of the already-masked
   x_valid/w_valid used for every other statistic in the same loop. np.min/
   np.max of a NaN-containing array is NaN, and np.histogram raises ValueError
   on a NaN range, so this crashed the entire results/plotting stage whenever
   any parameter had a NaN sample. The same histogram-plus-mode logic was
   duplicated (correctly, with masking) in the posterior distribution plot
   code a few hundred lines down, which is how the divergence was caught.
   Extracted a shared _weightedHistogramMode() helper used by both call
   sites, so there's now a single place responsible for this computation.

Test plan:
- Added wmpl/Dynesty/Tests/test_DynestyMetSim.py: 6 regression tests covering
  all four bugs. runSimulationDynesty/runSimulation/constructConstants are
  mocked out with hand-built SimpleNamespace stand-ins, so the suite runs in
  milliseconds with no real MetSim/dynesty evaluation.
- Verified each test fails against the pre-fix code (via git stash) and
  passes with the fixes applied.
- Run with: python -m wmpl.Dynesty.Tests.test_DynestyMetSim
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.

1 participant