Closed pierreroudier closed 2 years ago
I like this probs
option, that is handy and saves me writing code to do it myself each time. However, is there a need for the return.samples
option when generate()
will return this with almost exactly the same arguments as the predict()
call? Or is there some significant difference between what this returns and what generate()
does?
Thanks @pierreroudier ! The probs
part is a good addition; I think I'll take this opportunity to make a non-backwards-compatible change and make 0.5 return q0.5
instead of median
, to make the output more machine-readable (and also more like regular inla summary output), and remove some of the legacy outputs (I'll add an option to reproduce the old behaviour; we'll also re-add an old feature that provided the Monte Carlo uncertainties).
The samples aspects seems to just replicated the generate.bru
output, and would be better left out of predict.bru
.
The predict code can probably be cleaned up/reorganized (but that would be better done after merging the probs feature, as it's unrelated). It should ideally only need to do
... <- generate(...)
... <- bru_summarise(...)
but mostly for historical reasons it's a bit more messy than that; the main issue has been to create spatial object outputs.
I've also planned to add a broom::tidy
-like option.
I'll take a closer look at the PR and let you know and/or modify/merge.
Thanks for the feedback @ASeatonSpatial and @finnlindgren !
You are absolutely right, I got carried away when implementing the probs
option, and saw that (unused) cbind.only
option in bru_summarise
, and just went for it! Latest commit https://github.com/inlabru-org/inlabru/pull/143/commits/cf158f6378d8193229671069cc44472f25a857e2 removes return.samples
, but keeps probs
.
I think I'll take this opportunity to make a non-backwards-compatible change and make 0.5 return
q0.5
instead ofmedian
, to make the output more machine-readable (and also more like regular inla summary output)
Good idea I think, you'll see this is just a if
statement to remove/alter: https://github.com/pierreroudier/inlabru/blob/cf158f6378d8193229671069cc44472f25a857e2/R/bru.inference.R#L1436-L1438
I noticed from the checking action runs that the documentation needs some small fixes; note that we use the markdown-enabled roxygen style, so can/should use ...
instead of \code{...}, and also need to "protect" [0,1], with [0,1]
to prevent it from making it a link.
I noticed from the checking action runs that the documentation needs some small fixes; note that we use the markdown-enabled roxygen style, so can/should use
...
instead of \code{...}, and also need to "protect" [0,1], with[0,1]
to prevent it from making it a link.
My bad -- corrected in https://github.com/inlabru-org/inlabru/pull/143/commits/c8405e861db9d2dd6bdfb1695f44fd885939b047
This PR implements two simple options for
predict.bru
:probs
allows the user to pass custom probabilities (viabru_summarise
). It defaults toprobs = c(0.025, 0.5, 0.975)
, which is the current behaviour.return.samples
overrides theprobs
option, and return a matrix of samples. Effectively, it simply "wires in" the existingcbind.only
option ofbru_summarise
.Examples:
For the
probs
option:Which gives:
Note that for a probability of
0.5
, the column name is automatically changed tomedian
(current behaviour):Which gives:
The default behaviour is the same as the current behaviour:
Which gives:
For the
return.samples
option:Which gives: