Fix silent likelihood/reporting corruption in the Dynesty log-likelihood pipeline - #88
Open
AstroEloy wants to merge 1 commit into
Open
Fix silent likelihood/reporting corruption in the Dynesty log-likelihood pipeline#88AstroEloy wants to merge 1 commit into
AstroEloy wants to merge 1 commit into
Conversation
…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
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.
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:
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).
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.
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.
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:
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.
passes with the fixes applied.
🤖 Co-Authored-By: Claude Opus 4.8