greta-dev / greta-dev/greta

Is the dimension-repair branch in run_burst() still reachable?

Open
#837 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
607
Forks
67
Avg merge
3d 8h
Merged PRs (30d)
1

Description

## What the code says

[`R/sampler_class.R#L556-L560`](https://github.com/greta-dev/greta/blob/e8563dae0ef2b8be384fac023f62df1febf17ae6/R/sampler_class.R#L556-L560):

```r
# if there is one sample at a time, and it's rejected, conversion from
# python back to R can drop a dimension, so handle that here. Ugh.
if (n_dim(free_state_draws) != 3) {
dim(free_state_draws) <- c(1, dim(free_state_draws))
}
```

## What I measured

I could not make the drop happen. Calling
[`sample_carefully()`](https://github.com/greta-dev/greta/blob/e8563dae0ef2b8be384fac023f62df1febf17ae6/R/sampler_class.R#L588) directly with
`sampler_burst_length = 1L` and `hmc(epsilon = 1e10)` to force rejection, across
3 model shapes (scalar, `normal(dim = 5)`, two variables) x
{`epsilon = 0.1`, `1e10`} x {1, 3} chains:

**12 of 12 returned `ndim = 3`**, including every rejected single-sample case —
which is exactly the case the comment names.

```
scalar eps=1e+10 ch=1 dim=1x1x1 ndim=3 any_rejected=TRUE
vector[5] eps=1e+10 ch=1 dim=1x1x5 ndim=3 any_rejected=TRUE
2 variables eps=1e+10 ch=3 dim=1x3x2 ndim=3 any_rejected=TRUE
```

## Why this is worth an issue rather than a deletion

Twelve probes cannot prove a branch unreachable. Deciding this properly means
reasoning about what `tfp$mcmc$sample_chain` guarantees for the shape of
`all_states`, and what `reticulate` does converting it — not running more
cases.

Two things make it worth someone's time when they are next in `run_burst()`:

- If it **is** dead, it is TF1-era handling that survived the TF2 port, and the
"Ugh." is a standing invitation to leave it alone forever.
- If it is **not** dead, there is a reachable path where a burst returns
something other than a 3-D array, and nothing in the test suite covers it.
That is worth a test either way.

## A test for this

Whichever way it goes, the case the comment describes should be covered:

```r
test_that("a rejected single-sample burst still returns a 3d array", {
x <- normal(0, 1)
m <- model(x)
s <- greta:::build_sampler(
initial_values = greta:::prep_initials(initials(), 1, m$dag),
sampler = hmc(epsilon = 1e10), model = m, compute_options = cpu_only()
)
res <- s$sample_carefully(
free_state = s$free_state, sampler_burst_length = 1L,
sampler_thin = 1L, sampler_param_vec = unlist(s$sampler_parameter_values())
)
expect_false(as.array(res$trace$is_accepted)[[1]])
expect_length(dim(as.array(res$all_states)), 3)
})
```

Found sweeping the `TF1/2` markers for #745.

Contributor guide

Open the contributing guide

Research direction

Start in R/sampler_class.R, especially run_burst() and sample_carefully(), and review the linked dimension-repair branch and the proposed rejected single-sample test. Determine what tfp$mcmc$sample_chain guarantees for all_states and how reticulate converts it; done means either documenting the reachable path with coverage or removing the dead handling and its comment.

Written by the indexing model from the issue text.

Assessment

Tech stack
r, tensorflow
Domain
backend, testing
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.