add DV as variable to model() call & add bmm_formula #92
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #92 +/- ##
===========================================
+ Coverage 66.09% 68.57% +2.48%
===========================================
Files 14 14
Lines 926 1031 +105
===========================================
+ Hits 612 707 +95
- Misses 314 324 +10 ☔ View full report in Codecov by Sentry. |
…rences in documentation - there are still brms::bf cases in examples and vignettes, but let's change those at the end when everything is working
venpopov
force-pushed
the
79-DV-passed-to-model
branch
from
d462c6f to
5739124
Compare
- also add check for any incorrect parameters - reorder formula terms to match order in model info
- this makes it easier to combine different formulas into one, without having to code the names of the parameters every time. also allows users to build up a formula with bmm(c~1) + bmm(kappa~1), etc, if needed.
- also add a subsetting method such that we can extract any parts of a bmmformula. E.g.
f1 <- bmm(c~1, kappa~1, a~1)
identical(f1[c('c','kappa')], bmm(c~1, kappa~1))
this also allows to easily reorder terms in a formula - I've made it so it now orders the terms as they are ordered in the model
- new function message2() only prints message if bmm.silent < 2 (corresponding to the brms silent argument - generalize configure_options
- i realized that automatically excluding arguments that are not named arguments for brms would exclude arguments that you can pass through ... from brms to rstan or cmdstanr, so we have to exclude bmm specific arguments manually
- this is to avoid error with adding automatically to formula even though it is fixed and cannot be predicted
|
I have now also gone through all the changed files listed here on GitHub and had no further comments beyond the small commits that I made for the |
|
I fixed the setsize checks. I want to ad a few more tests and finish documentation of new functions, which I'll do Saturday. You could look into updating the examples for all functions. Can you also describe the main changes and motivation in the main message for the pull request for the record, and update the news.md with the same info? |
Yes, definetely. Actually, I will have a look into saving the examples in a separate Examples folder and come up with a unified naming scheme, if you are ok with this. I felt that this might be easier than having the R-Code in the commented format. This way it would also be easier to check that all the examples are running with the current version of the package. I assume there should be an option in the checks to verify that there are no errors in the examples. |
|
Sure, though does that still work with {don't run}? We don't want to run all models every time we build the package locally with check() |
- change documentation accordingly - update readme to reflect relabeling & new `bmmformula` syntax
- updated `use_model_template` to reflect the new `bmmformula` syntax - additional updates to the documentation
… into 79-DV-passed-to-model # Conflicts: # R/bmm_model_IMM.R # R/helpers-data.R # tests/testthat/test-fit_model.R # tests/testthat/test-helpers-data.R
NEWS.md
Outdated
| ### New features | ||
|
|
||
| * BREAKING CHANGE: The `fit_model` function now requires a `bmmformula` to be passed. The syntax of the `bmmformula` or its short form `bmf` is equal to specifying a `brmsformula`. However, as of this version the `bmmformula` only specifies how parameters of a `bmmmodel` change across experimental conditions or continuous predictors. The response variables that the model is fit to now have to be specified when the model is defined using `model = bmmmodel()`. (#79) | ||
| * BREAKING CHANGE: The `non_target` and `spaPos` variables for the `mixture3p` and `IMM` models were relabled to `nt_features` and `nt_distance` for consistency. This is also to communicate that distance is not limited to spatial distance but distances on any feature dimensions of the retrieval cues. Currently, still only a single generalization gradient for the cue features is possible. |
There was a problem hiding this comment.
let's make them both plural for consistency - especially since both arguments take a vector of variable names for features or distances of each non_target variable. I'll make that change everywhere
NEWS.md
Outdated
| @@ -35,7 +52,7 @@ | |||
|
|
|||
| ### New features | |||
|
|
|||
| * BREAKING CHANGE: Improve user interface to fit_model() ensures package stability and future development. Model specific arguments are now passed to the model functions as named arguments (e.g. `mixture3p(non_targets, setsize)`). This allows for a more flexible and intuitive way to specify model arguments. Passing model specific arguments directly to the `fit_model()` function is now deprecated (#43). | |||
| * BREAKING CHANGE: Improve user interface to fit_model() ensures package stability and future development. Model specific arguments are now passed to the model functions as named arguments (e.g. `mixture3p(nt_features, setsize)`). This allows for a more flexible and intuitive way to specify model arguments. Passing model specific arguments directly to the `fit_model()` function is now deprecated (#43). | |||
There was a problem hiding this comment.
since this release exists and can be installed, we should not rewrite history as it can be confusing. I'll change this back (though I suspect this might have been find and replace's fault :)
There was a problem hiding this comment.
Yes, this was find and replace. But I totally, agree that we should not change the NEWS of past releases. I will be more careful the next time I use find and replace across all files -.-
- the arguments used to fit the bmm model are now accessible in the `bmmfit` object via the `fit$bmm_fit_args` list
- remove all unnecessary returns at the end of functions for style consistency
- other minor stylistic changes
- add class ('bmmfit')
There was a problem hiding this comment.
this file uses version v0.2.1 for generating ref fits, so the syntax should not be changed to the new one (also the formula is still the old one so the change is inconsistent. I will revert it and then name the file with the bmm version for generating the ref fits. We can have different versions of this file for different bmm version ref fits
- I had changed this sdmSimple recently, because the old method introduced unnecessary characters which forced recompilation of the model even if it was unnecessary
- also note when developers can delete sections of use_model_template
|
OK, this should be good to go. I will check the dev-notes and after that you can merge this |
| " data = NextMethod('check_data')\n\n", | ||
| " return(data)\n", | ||
| "}\n\n", | ||
| .open = "<<", .close = ">>") | ||
|
|
||
| # add bmf2bf method if necessary | ||
| bmf2bf_method <- glue::glue("#' @export\n", |
There was a problem hiding this comment.
This comment is for the future, no need to do anything for this pull request.
You know more about models that use more than one response variable. Once you are done with EZDM, can we generalize this function to have a bmf2bf.2resp? Or are there many different ways to specify 2 response variables depending on the type of model.
A second note is that there is code duplication from the other method. everything is the same except for the reponse part of the formula. So once you have the ezdm merged, we can see what can be refactored.
| model <- IMMfull(non_targets = paste0("Item",2:8,"_Col"), | ||
| spaPos = paste0("Item",2:8,"_Pos")) | ||
| model <- IMMfull(nt_features = paste0("Item",2:8,"_Col"), | ||
| nt_distance = paste0("Item",2:8,"_Pos")) | ||
|
|
||
| fit <- fit_model( |
There was a problem hiding this comment.
Similar to the comment you made about re-labeling the non_targets and spaPos, it occurred to me that we could also think about relabeling the fit_model function to bmm in the sense that this function specifies and estimates one of the "Bayesian Measurement Models". I was thinking if it could be confusing that the package name and its main function are the same, but that in itself is not uncommon. For example, the lavaan package also has a lavaan function that is called for all Structural Equation Models estimated in the package.
For the start, we could also make an alias so that fit_model and bmm work both.
NEWS.md
Outdated
| @@ -35,7 +52,7 @@ | |||
|
|
|||
| ### New features | |||
|
|
|||
| * BREAKING CHANGE: Improve user interface to fit_model() ensures package stability and future development. Model specific arguments are now passed to the model functions as named arguments (e.g. `mixture3p(non_targets, setsize)`). This allows for a more flexible and intuitive way to specify model arguments. Passing model specific arguments directly to the `fit_model()` function is now deprecated (#43). | |||
| * BREAKING CHANGE: Improve user interface to fit_model() ensures package stability and future development. Model specific arguments are now passed to the model functions as named arguments (e.g. `mixture3p(nt_features, setsize)`). This allows for a more flexible and intuitive way to specify model arguments. Passing model specific arguments directly to the `fit_model()` function is now deprecated (#43). | |||
There was a problem hiding this comment.
Yes, this was find and replace. But I totally, agree that we should not change the NEWS of past releases. I will be more careful the next time I use find and replace across all files -.-
Summary
This commit moves away from using brmsformulas to be supplied to the fit_model function. Additionally, the dependent varible needs to be specified in the model call and is no longer part of the model formula.
Closes #98.
Tests
[x] Confirm that all tests passed
[x] Confirm that devtools::check() produces no errors
Release notes
New features
fit_modelfunction now requires abmmformulato be passed. The syntax of thebmmformulaor its short formbmfis equal to specifying abrmsformula. However, as of this version thebmmformulaonly specifies how parameters of abmmmodelchange across experimental conditions or continuous predictors. The response variables that the model is fit to now have to be specified when the model is defined usingmodel = bmmmodel(). (require dependent variable to be passed to the model to avoid specifying it in the formula #79)non_targetandspaPosvariables for themixture3pandIMMmodels were relabled tont_featuresandnt_distancesfor consistency. This is also to communicate that distance is not limited to spatial distance but distances on any feature dimensions of the retrieval cues. Currently, still only a single generalization gradient for the cue features is possible.check_formulamethods have been adapted to match the newbmmformulasyntax. It now evaluates if formulas have been specified using thebmmformulafunction, if formulas for all parameters of abmmmodelhave been specified and warns the user that only a fixed intercept will be estimated if no formula for one of the parameters was provided. Additionally,check_formulathrows an error should formulas be provided that do not match a parameter of the calledbmmmodelunless they are part of a non-linear transformation.muin all visual working memory models. This allows you to predict if there is response bias in the data. If a formula is not provided formu, the model will assume that the mean of the response distribution is fixed to zero.bmm.silentthat allows to suppress messagesbwas removed from theIMMmodels, as this is internally fixedto zero for scaling and as of now cannot be predicted by independent variables because the model would be unidentifiable.
bmmfitobject via thefit$bmm_fit_argslist.brmsfitobject. The object is now of class('bmmfit', 'brmsfit')Bug Fixes
IMMfulland theIMMbschas been corrected. This versions ensures that only positive distances can be passed to any of the two models.IMMfulland theIMMbscthat was specific only for circular distances.Documentation
bmmformulasyntax.