Proposal: transform get_prior, make_stancode and make_standata into S3 methods #1604
Conversation
|
I like the idea. my suggestion would be to just make the current function
the .default method. that should be doable with just changing a few lines
of code.
Ven Popov ***@***.***> schrieb am Mi., 28. Feb. 2024, 15:48:
… Hi @paul-buerkner <https://github.com/paul-buerkner>, related to the
recent discussion on the Stan forums
<https://discourse.mc-stan.org/t/flocker-an-r-package-for-occupancy-modeling-with-brms/34132/8?u=ven_popov>
about packages that build on top of brms, I'd like to propose the
following.
Proposal
Transform get_prior(), make_stancode() and make_standata() into generic S3
functions, with the current functions assigned to a
get_prior.brmsformula(), make_stancode.brmsformula() and
make_standata.brmsformula() (and equivalent aliases for .formula and
.mvbrmsformula)
Why?
Some feature shared by a few of the packages that build on top of brms in
the list I posted, is that they:
- generate custom stancode to pass to brms
- provide default priors for their models
- define a custom formula class (bmmformula, brm_formula
To deal with the above, some package also provide analogues to brms`s
get_prior(), make_stancode() and make_standata() that work with the models,
formulas and data transformation they have provided.
For example, in bmm, we have:
- get_model_prior(formula, data, model,...)
<https://github.com/venpopov/bmm/blob/bcb9d7ee6740d7cf1554f0618e1a9a93cf835da9/R/helpers-prior.R#L67>
function, which does all the preprocessing we need to generate a brms
appropriate objects, and then calls brms::get_prior.
- get_stancode(formula, data, model,...)
<https://github.com/venpopov/bmm/blob/bcb9d7ee6740d7cf1554f0618e1a9a93cf835da9/R/helpers-model.R#L654>
which does the same, but eventually calls brms::make_stancode
- similarly for get_standata
Similarly, @jsocolar <https://github.com/jsocolar>'s flocker package has
analogues get_flocker_prior() and flocker_stancode()
<jsocolar/flocker#69>, that also take a formula
as the first argument. This is the case in some other packages, but I
haven't made a full list yet.
I've been thinking about more general package development based on brms,
and this would be really streamlined if some of the user-facing functions
like those mentioned, can be used across packages in a consistent way - to
allow a general development framework, and to make it easier for users.
We were thinking of overloading these methods
<venpopov/bmm#132> within bmm by providing a
get_prior.default(), make_stancode.default() etc methods that call the
current brms functions, so that when users load our package, they can still
use the get_prior() with both brms and bmm.
This is made possible by the fact that we have a custom formula class,
something present in other similar packages.
But rather than us overloading brms functions, I think it would be nicer,
and more sustainble and generalizable, to transform these functions
natively within brms
Example implementation
In this PR I've implemented as an example the S3 method for the
get_prior() function. I have tested it both within brms and bmm and all
the existing tests pass - when called with a formula, brmsformula or
mvbrmsformula object, it applies the brms get_prior function, and when it
is applied with a bmmformula object, it applies our method.
Let me know what you think - If you approve of the idea, I can do the
other functions too.
Towards a meta-development framework around brms
With these changes, it becomes easier to work towards a broader
development framework for packages that build on top of brms. A package
of this type would generally have:
- A custom pkgformula class, which allows all these methods to be
adjusted as needed (in our case, bmmformula, but I've seen bayesnecformula,
brm_formula, and a few others). Then they have some internal methods that
translate their subject-matter formulas into valid brmsformulas
- A custom pkgfit class, which is applied in addition to the brmsfit
class, allowing for customizing the base generic functions such as
summary, update, etc. For example, in bmm we needed to write a new
update method, to make sure that all the transofmrations and preprocessing
is reapplied before submitting to update.brmsfit.
- A set of internal functions and objects that:
- take the pkgformula, pkgfamily, the user data, to generate valid
brms arguments, including setting default priors informed by subject
knowledge, transformations of the data.
image.png (view on web)
<https://github.com/paul-buerkner/brms/assets/12783882/5d4051b1-a7ba-443e-a650-0e4db2810b92>
------------------------------
You can view, comment on, or merge this pull request online at:
#1604
Commit Summary
- cdccc21
<cdccc21>
transform get_prior to a generic S3 method
File Changes
(5 files <https://github.com/paul-buerkner/brms/pull/1604/files>)
- *M* DESCRIPTION
<https://github.com/paul-buerkner/brms/pull/1604/files#diff-9cc358405149db607ff830a16f0b4b21f7366e3c99ec00d52800acebe21b231c>
(4)
- *M* NAMESPACE
<https://github.com/paul-buerkner/brms/pull/1604/files#diff-b6b6854a02ae177f8860f49654ff250c1554d021ae434b6efdbe99d00bdc6055>
(3)
- *M* R/priors.R
<https://github.com/paul-buerkner/brms/pull/1604/files#diff-5c5938f103d3a8f1b54a03814c277ef6e1e0c88ee21ee58a86edf2f1a129ab5f>
(51)
- *M* man/get_prior.Rd
<https://github.com/paul-buerkner/brms/pull/1604/files#diff-7b737b0d20a4a80bd58fff3d33c701ad6d3d0bc3e12e50da488e9bc0b1066b77>
(91)
- *A* man/get_prior.brmsformula.Rd
<https://github.com/paul-buerkner/brms/pull/1604/files#diff-b3263101a3fb221f99d0fd52e98e5bde9827458e75b5ac314328b4b891fd6f82>
(97)
Patch Links:
- https://github.com/paul-buerkner/brms/pull/1604.patch
- https://github.com/paul-buerkner/brms/pull/1604.diff
—
Reply to this email directly, view it on GitHub
<#1604>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADCW2AGA7WLCXUCSVEMDSULYV47TPAVCNFSM6AAAAABD6FKTLOVHI2DSMVQWIX3LMV43ASLTON2WKOZSGE2TSMJUG4ZTMNY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Cool! Yes, I initially had it coded like that, wasn't sure whether you'd prefer it to be more explicit. If the current function is the default, in principle there is no need for the aliases *.formula, *.brmsformula and *.mvbrmsformula, as all would be handled by the default. Or do you prefer to have them explicitly handled as aliases? I can change that and apply it to the other functions as well |
|
Only the default method. No aliases |
- use \code{\link[brms:get_prior.default]{get_prior}} ueverywhere where [brms:get_prior] was previously used to ensure links point to the default method documentation rather than the S3 method
- assign previous function to make_standata.default
- update links in docs to \code{\link[brms:make_standata.default]{make_standata}
- assign previous function to make_stancode.default
- update links in docs to \code{\link[brms:make_stancode.default]{make_stancode}
- fix typo on line99 of make_standata.R incorrectly referring to make_stancode instead of make_standata
|
Ok, should be done now! Do you want me to add an entry to NEWS.md if you approve the changes? Following the example of how you handled documentation of other methods such as summary and update, I replaced all links from other docs to and equivalent for the other two functions. That way it displays the name of the generic, but it links to the documentation of the brms default, so users always get that info I also tested it with the now updated methods in our package, and it works great! 😃 |
|
Great, thank you! I will check it out in the next couple of days and make perhaps make some tiny edits before merging. Will then also update NEWS. |
|
I have worked on this PR further and decided to make a bit more bold changes. Specifically, I changeed I would appericiate if you could try it out and let me know if everything is working as expected. Also, it would be nice if you checked my changes to the documentation (consistency, correct links etc.) Before merging, I will then make some more edits such as changes some examples, tests, and file names, but nothing that is more than a search and replace. |
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1604 +/- ##
==========================================
+ Coverage 82.02% 82.06% +0.03%
==========================================
Files 70 70
Lines 19888 19886 -2
==========================================
+ Hits 16314 16320 +6
+ Misses 3574 3566 -8 ☔ View full report in Codecov by Sentry. |
|
OK, will do today |
|
Done. And tested it with changes to our package, it works. Thanks! The only things that I didn't change were:
Maybe you also want to rename the R files to stancode.R and standata.R? Up to you |
|
Great! Will make the last changes and then merge. I will do the file renaming on main after merging so we see the actual changes in this PR |
Hi @paul-buerkner, related to the recent discussion on the Stan forums about packages that build on top of
brms, I'd like to propose the following.Proposal
Transform get_prior(), make_stancode() and make_standata() into generic S3 functions, with the current functions assigned to a get_prior.brmsformula(), make_stancode.brmsformula() and make_standata.brmsformula() (and equivalent aliases for .formula and .mvbrmsformula)
Why?
Some feature shared by a few of the packages that build on top of
brmsin the list I posted, is that they:brmsTo deal with the above, some package also provide analogues to brms`s get_prior(), make_stancode() and make_standata() that work with the models, formulas and data transformation they have provided.
For example, in
bmm, we have:Similarly, @jsocolar's flocker package has analogues get_flocker_prior() and flocker_stancode(), that also take a formula as the first argument. This is the case in some other packages, but I haven't made a full list yet.
I've been thinking about more general package development based on brms, and this would be really streamlined if some of the user-facing functions like those mentioned, can be used across packages in a consistent way - to allow a general development framework, and to make it easier for users.
We were thinking of overloading these methods within
bmmby providing a get_prior.default(), make_stancode.default() etc methods that call the current brms functions, so that when users load our package, they can still use the get_prior() with bothbrmsandbmm.This is made possible by the fact that we have a custom formula class, something present in other similar packages.
But rather than us overloading brms functions, I think it would be nicer, and more sustainble and generalizable, to transform these functions natively within
brmsExample implementation
In this PR I've implemented as an example the S3 method for the get_prior() function. I have tested it both within
brmsandbmmand all the existing tests pass - when called with a formula, brmsformula or mvbrmsformula object, it applies the brms get_prior function, and when it is applied with a bmmformula object, it applies our method.Let me know what you think - If you approve of the idea, I can do the other functions too.
Towards a meta-development framework around
brmsWith these changes, it becomes easier to work towards a broader development framework for packages that build on top of
brms. A package of this type would generally have:pkgformulaclass, which allows all these methods to be adjusted as needed (in our case, bmmformula, but I've seen bayesnecformula, brm_formula, and a few others). Then they have some internal methods that translate their subject-matter formulas into valid brmsformulaspkgfitclass, which is applied in addition to thebrmsfitclass, allowing for customizing the base generic functions such assummary,update, etc. For example, inbmmwe needed to write a new update method, to make sure that all the transofmrations and preprocessing is reapplied before submitting to update.brmsfit.pkgformula,pkgfamily, the user data, to generate valid brms arguments, including setting default priors informed by subject knowledge, transformations of the data.