Filter non-finite de_sample() draws in sample() for unbounded priors too - #245
Merged
Conversation
sample.nsbi_posterior()'s non-finite-row filter ran only inside `if (bounded)`, since it was added by #234/#236 specifically to work around within_support() returning NA (not FALSE) for a NaN row. That left the common case -- an unbounded prior like prior_normal() -- with no filter at all: a NaN/Inf row from de_sample() (which MAF/NSF/MDN can occasionally produce) was rbind'd straight into the returned draws matrix, and acceptance_rate still reported 1.0. Drop non-finite rows unconditionally, before the bounded branch's within_support() rejection runs, so acceptance_rate reflects the drop for every prior. log_prob.nsbi_posterior()'s normalize = TRUE path needs no matching change: its de_sample() call for the acceptance estimate is itself gated on `bounded`, so it never runs (and never could see a non-finite draw) in the unbounded case. Fixes #244.
…finite-draw # Conflicts: # NEWS.md
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #245 +/- ##
=======================================
Coverage 98.38% 98.38%
=======================================
Files 31 31
Lines 4034 4035 +1
=======================================
+ Hits 3969 3970 +1
Misses 65 65 ☔ View full report in Codecov by Harness. |
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.
Fixes #244
Bug
sample.nsbi_posterior()(R/posterior.R) only filters a non-finite draw from the density estimator insideif (bounded) { ... }, wherebounded <- !is.null(prior$lower) || !is.null(prior$upper). That guard was added by #234/#236 specifically to work aroundwithin_support()returningNA(notFALSE) for a NaN row, and R's matrix indexing keeping rather than dropping a row selected by anNAlogical index.The guard never runs for an unbounded prior (
prior_normal(), or anyprior_custom()/family withoutlower/upper) -- the common case; most of the package's own NPE examples useprior_normal(). For an unbounded prior, a NaN/Inf row that an under-trained MAF/NSF/MDN can produce wasrbind'd straight into the returned draws matrix with no warning, andattr(draws, "acceptance_rate")still reported1.0. This silently corruptssummary(),pairplot(), andsbc()/tarp()diagnostics built onsample(), and can surface downstream inmap_estimate()as a confusing "thetacontains non-finite value" error that blames the seed draw rather than the internal density-estimator artifact that produced it.Fix
Moved the non-finite-row filter out of
if (bounded)so it runs unconditionally, right afterde_sample(), using a plainis.finite()row check independent ofwithin_support(). The existingwithin_support()/rejection-sampling logic stays scoped toboundedexactly as before -- this is additive, not a replacement. Becausecollectedis filtered beforeacceptance_rate <- nrow(collected) / max(n_tried, 1)is computed, the dropped row is reflected inacceptance_ratefor every prior, bounded or not, with no separate accounting needed.log_prob.nsbi_posterior()'snormalize = TRUEpath needs no matching change: itsde_sample()call for the acceptance-constant estimate is itself gated onnormalize && bounded, so for an unbounded prior that call -- and any chance of it returning a non-finite draw -- never happens in the first place.de_log_prob()is called unconditionally in that function, but only on user-suppliedtheta, which is already run throughcheck_finite()earlier in the same function; it isn't fed draws fromde_sample(), so it needs no guard here.Tests
Added two regression tests to
tests/testthat/test-posterior-nonfinite-de-draw.R(mirroring the existing #234/#236 tests, but withprior_normal()instead ofprior_uniform()):sample()drops a mocked NaN row fromde_sample()and returns the requestednfinite draws, running a second sampling batch to make it up.sample()warns about the shortfall andacceptance_ratecorrectly reports0, not the pre-fix1.0.Checks
R CMD INSTALL --no-docs . && (cd tests && Rscript testthat.R):FAIL 0 | WARN 0 | SKIP 79 | PASS 1373(torch unavailable in this sandbox, so neural/MDN/MAF/NSF tests skip, as documented in CLAUDE.md; all validation-path tests, including the new ones, ran and passed).DESCRIPTIONbumped to 0.6.20,NEWS.mdentry added (PR number filled in once assigned).man/*.Rdneeds no update.Generated by Claude Code