From 8a9551cc4300eabd5f4bf77ab6af275027b28674 Mon Sep 17 00:00:00 2001 From: Pedro Lima Date: Fri, 28 Aug 2026 07:00:56 +0000 Subject: [PATCH 1/2] Filter non-finite de_sample() draws in sample() for unbounded priors too 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. --- DESCRIPTION | 2 +- NEWS.md | 4 ++ R/posterior.R | 12 +++- .../test-posterior-nonfinite-de-draw.R | 67 +++++++++++++++++++ 4 files changed, 83 insertions(+), 2 deletions(-) diff --git a/DESCRIPTION b/DESCRIPTION index 7409c41d..79fc87a5 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -1,6 +1,6 @@ Package: neuralsbi Title: Neural Simulation-Based Inference -Version: 0.6.19 +Version: 0.6.20 Authors@R: person("Pedro", "Nascimento de Lima", email = "plima@rand.org", role = c("aut", "cre"), comment = c(ORCID = "0000-0001-9057-198X")) diff --git a/NEWS.md b/NEWS.md index c6e2cd36..a4078f9b 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,3 +1,7 @@ +# neuralsbi 0.6.20 + +* **`sample()` on an unbounded posterior (e.g. `prior_normal()`) no longer lets a non-finite draw from the density estimator through.** `sample.nsbi_posterior()`'s non-finite-row filter ran only inside `if (bounded)`, since it was added by #234/#236 to work around `within_support()` returning `NA` (not `FALSE`) for a NaN row -- a problem specific to the bounded rejection-sampling path. That left `bounded <- !is.null(prior$lower) || !is.null(prior$upper)` `FALSE` for an unbounded prior, the common case in the package's own NPE examples, with no filter at all: a NaN/Inf row that an under-trained MAF/NSF/MDN occasionally produces was `rbind`'d straight into the returned draws matrix, `attr(draws, "acceptance_rate")` still reported `1.0`, and the corruption propagated silently into `summary()`, `pairplot()`, and `sbc()`/`tarp()` diagnostics -- or surfaced downstream in `map_estimate()` as a confusing "`theta` contains non-finite value" error blaming the seed draw. `sample.nsbi_posterior()` now drops a non-finite row from `de_sample()`'s output unconditionally, before the `bounded` branch's `within_support()` check runs, so `acceptance_rate` reflects the drop for every prior (#244) (#XXX). + # neuralsbi 0.6.19 * **`nre()` no longer silently trains on zero gradient when the training split, not just the validation split, drops to one row.** `check_train_controls()` (`R/train.R`) enforced `min_val_rows` (2 for `nre()`'s atomic contrastive objective, per #188) against `n_val` only, with no matching floor on `n_tr = n - n_val`. A large `validation_fraction` can clear the validation-side floor while leaving `n_tr` below it -- `n_simulations = 4`, `validation_fraction = 0.75` gives `n_val = 3` (passes) and `n_tr = 1` (was never checked). `train_restarts()` then trained on that single row, and `nre_atomic_log_prob()`'s `k < 2L` guard -- the same branch #188 fixed for the validation side -- returned a constant zero loss every step: no gradient, no error, training ran to `patience` epochs and reported a `best_val_loss` as if it had actually trained. `check_train_controls()` now also requires `n - n_val >= min_val_rows`, the same floor already applied to the validation side (#239) (#242). diff --git a/R/posterior.R b/R/posterior.R index d0aaa09b..2e4ab1a2 100644 --- a/R/posterior.R +++ b/R/posterior.R @@ -165,13 +165,23 @@ sample.nsbi_posterior <- function(x, size = 1000, n = size, obs = NULL, draw_std <- de_sample(fit$de, xo_std, n_needed) draw <- invert_standardizer(fit$std_theta, draw_std) n_tried <- n_tried + n_needed + # A non-finite row from de_sample() (an under-trained MAF/NSF/MDN can + # produce one) needs dropping regardless of whether the prior is bounded + # -- unlike within_support()'s NA-vs-FALSE issue below, this filter has + # nothing to do with support and ran only inside `if (bounded)` before + # #244, so an unbounded prior (prior_normal(), the common case) let a + # NaN/Inf draw straight into the returned matrix with acceptance_rate + # still reporting 1.0. + draw <- draw[apply(is.finite(draw), 1, all), , drop = FALSE] if (bounded) { # within_support() returns NA for a NaN/NA row, and R's matrix indexing # keeps (rather than drops) a row selected by an NA logical index and # fills it with NA -- so a NaN draw from the density estimator would # otherwise survive as an all-NA row counted toward n (#234). Coerce NA # to FALSE so a non-finite draw is rejected the same way an - # out-of-bounds one is. + # out-of-bounds one is. (The finite-row filter above already removes + # non-finite rows, so this NA never actually arises here anymore, but + # within_support() is still the source of truth for the bound itself.) ok <- within_support(prior, draw) ok[is.na(ok)] <- FALSE draw <- draw[ok, , drop = FALSE] diff --git a/tests/testthat/test-posterior-nonfinite-de-draw.R b/tests/testthat/test-posterior-nonfinite-de-draw.R index 8564ee3d..0aa3d1d9 100644 --- a/tests/testthat/test-posterior-nonfinite-de-draw.R +++ b/tests/testthat/test-posterior-nonfinite-de-draw.R @@ -9,6 +9,14 @@ # NaN draw the way an under-trained MAF/NSF/MDN might, and check that a # non-finite draw is rejected rather than silently accepted or crashing with # base R's unrelated "missing value where TRUE/FALSE needed". +# +# GitHub #244: the fix above only ran inside `if (bounded)`, since #234's +# NA-vs-FALSE problem is specific to within_support(). That left the common +# case -- an unbounded prior, e.g. prior_normal() -- with no filter at all: a +# NaN/Inf row from de_sample() went straight into sample()'s returned matrix, +# and attr(draws, "acceptance_rate") still reported 1.0. The tests below +# repeat the sample() case with prior_normal() instead of prior_uniform() to +# cover the unbounded path. test_that("sample() rejects a NaN draw from the density estimator instead of returning it", { set.seed(30) @@ -87,3 +95,62 @@ test_that("map_estimate()'s objective does not crash when queried at a non-finit expect_equal(queried, Inf) expect_true(within_support(prior, matrix(map, nrow = 1))) }) + +test_that("sample() rejects a NaN draw from the density estimator with an unbounded prior (#244)", { + set.seed(33) + prior <- prior_normal(mean = 0.5, sd = 1) + simulator <- function(theta) theta + stats::rnorm(1, sd = 0.05) + fit <- npe(prior, simulator, n_simulations = 500, + density_estimator = "linear_gaussian") + post <- posterior(fit, x_obs = 0.5) + + # Same technique as the bounded-prior test above, but prior_normal() has no + # `lower`/`upper`, so bounded is FALSE and the fix has to filter without + # relying on within_support() at all. + real_de_sample <- de_sample + call_count <- 0L + local_mocked_bindings( + de_sample = function(de, x, n) { + call_count <<- call_count + 1L + draw <- real_de_sample(de, x, n) + if (call_count == 1L) draw[1, ] <- NaN + draw + } + ) + + draws <- sample(post, n = 50, max_sampling_batches = 10) + expect_equal(nrow(draws), 50L) + expect_false(anyNA(draws)) + # without the fix, the NaN row is kept (not dropped), so round one alone + # would already satisfy n and no second round would run + expect_gt(call_count, 1L) +}) + +test_that("sample()'s acceptance_rate reflects a dropped non-finite row with an unbounded prior (#244)", { + set.seed(34) + prior <- prior_normal(mean = 0.5, sd = 1) + simulator <- function(theta) theta + stats::rnorm(1, sd = 0.05) + fit <- npe(prior, simulator, n_simulations = 500, + density_estimator = "linear_gaussian") + post <- posterior(fit, x_obs = 0.5) + + # Every draw in the one and only batch is corrupted, so n_needed rows are + # tried and none survive the filter -- acceptance_rate must report that + # rather than the pre-fix 1.0, and sample() should warn about the shortfall + # the same way it does for a bounded prior leaking mass. + real_de_sample <- de_sample + local_mocked_bindings( + de_sample = function(de, x, n) { + draw <- real_de_sample(de, x, n) + draw[] <- NaN + draw + } + ) + + expect_warning( + draws <- sample(post, n = 20, max_sampling_batches = 1), + "0/20 samples inside prior support" + ) + expect_equal(nrow(draws), 0L) + expect_equal(attr(draws, "acceptance_rate"), 0) +}) From 879cc57e04db8269aa282d9b2d47ce33dc0dbeb5 Mon Sep 17 00:00:00 2001 From: Pedro Lima Date: Fri, 28 Aug 2026 07:01:22 +0000 Subject: [PATCH 2/2] Add PR link to NEWS.md entry for #244 --- NEWS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index a4078f9b..014986eb 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,6 +1,6 @@ # neuralsbi 0.6.20 -* **`sample()` on an unbounded posterior (e.g. `prior_normal()`) no longer lets a non-finite draw from the density estimator through.** `sample.nsbi_posterior()`'s non-finite-row filter ran only inside `if (bounded)`, since it was added by #234/#236 to work around `within_support()` returning `NA` (not `FALSE`) for a NaN row -- a problem specific to the bounded rejection-sampling path. That left `bounded <- !is.null(prior$lower) || !is.null(prior$upper)` `FALSE` for an unbounded prior, the common case in the package's own NPE examples, with no filter at all: a NaN/Inf row that an under-trained MAF/NSF/MDN occasionally produces was `rbind`'d straight into the returned draws matrix, `attr(draws, "acceptance_rate")` still reported `1.0`, and the corruption propagated silently into `summary()`, `pairplot()`, and `sbc()`/`tarp()` diagnostics -- or surfaced downstream in `map_estimate()` as a confusing "`theta` contains non-finite value" error blaming the seed draw. `sample.nsbi_posterior()` now drops a non-finite row from `de_sample()`'s output unconditionally, before the `bounded` branch's `within_support()` check runs, so `acceptance_rate` reflects the drop for every prior (#244) (#XXX). +* **`sample()` on an unbounded posterior (e.g. `prior_normal()`) no longer lets a non-finite draw from the density estimator through.** `sample.nsbi_posterior()`'s non-finite-row filter ran only inside `if (bounded)`, since it was added by #234/#236 to work around `within_support()` returning `NA` (not `FALSE`) for a NaN row -- a problem specific to the bounded rejection-sampling path. That left `bounded <- !is.null(prior$lower) || !is.null(prior$upper)` `FALSE` for an unbounded prior, the common case in the package's own NPE examples, with no filter at all: a NaN/Inf row that an under-trained MAF/NSF/MDN occasionally produces was `rbind`'d straight into the returned draws matrix, `attr(draws, "acceptance_rate")` still reported `1.0`, and the corruption propagated silently into `summary()`, `pairplot()`, and `sbc()`/`tarp()` diagnostics -- or surfaced downstream in `map_estimate()` as a confusing "`theta` contains non-finite value" error blaming the seed draw. `sample.nsbi_posterior()` now drops a non-finite row from `de_sample()`'s output unconditionally, before the `bounded` branch's `within_support()` check runs, so `acceptance_rate` reflects the drop for every prior (#244) (#245). # neuralsbi 0.6.19