Merged
Conversation
Updated the all_vars function to accept formulas, lists of formulas, and character strings, improving flexibility and simplifying its usage throughout the codebase. Adjusted all relevant function calls and documentation to match the new interface, and updated tests to reflect the new expected behavior.
Incremented the package version to mark development changes. Updated NEWS.md with details about improvements to the all_vars() function
refactoring: using new version of all_vars()
Replaces all uses of extract_id with extract_grouping in R source files and updates related tests and documentation. Removes extract_id documentation and adds extract_grouping documentation.
Dropping unused factor levels with the help of drop_levels() was (accidentally?) de-actived in 35c1c27. This commit re-activates the check and produces a warning but doesn't automatically remove the unused factor levels.
new helper function to be used in the new version of get_nranef()
Added a new helper function, used in get_hc_info(), that returns a list of random effects specifications (one-sided formulas) by grouping level
Simplification of the function
the old, complicated remove_grouping() is not split into remove_formula_grouping(), which does the main work, and remove_grouping(), which is a wrapper for lists of formulas. Different compared to the old version is that the output is no longer named by the hierarchical levels because this was inconsistent in case a formula used multiple hierarchical levels (e.g., id/center)
Gave error run in CMD check because the function could not be found.
Refactor the functions used to check the input data and, if needed, reformat them. Add documentation to these (internal) functions.
the new version of convert_variables() does not have the "data_orig" argument anymore. The check in predict() to compare "newdata" to the data used for fitting the model should probably be done with compare_data_structure().
snapshot files, were automatically generated, probably due to first run on (local) MacOS?
…d added corresponding test.
dQuote() has an argument that has as the default a global option. Setting the value at the beginning of the test solved a failing RMD check due to different style quotes.
replace the new style of defining functions (\()) with the old function() to avoid restriction on the R version
make it possible to use the same function with different number of variables involved in the same model formula
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors helper functions for formula handling, particularly renaming extract_id() to extract_grouping(), extract_lhs() to extract_lhs_string(), and extract_outcome() to extract_outcomes_list(). The changes also update all_vars() to handle multiple input types, improve data checking functions, and fix a bug with using the same function type multiple times with different numbers of variables in formulas.
Key changes:
- Refactored formula helper functions with more descriptive names
- Enhanced
all_vars()to accept variable-length arguments including formulas and character strings - Fixed bug allowing repeated use of functions like
I()with different variable counts in formulas
Reviewed Changes
Copilot reviewed 54 out of 56 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/testthat/test-helpfunctions_formulas.R | Updates test data structures for remove_grouping() tests; contains syntax errors with parentheses instead of list() |
| tests/testthat/test-helpfunctions_formulas_general.R | Adds comprehensive tests for refactored functions; contains typos in test descriptions |
| tests/testthat/test-helpfunctions_vcov.R | Removes idvar parameter from get_nranef() calls |
| tests/testthat/test-helpfunctions_checks.R | New test file for data checking functions |
| tests/testthat/test-helpfunctions.R | Updates function name from extract_outcome() to extract_outcomes_list() |
| tests/testthat/test-groupings.R | New test file for grouping-related functions |
| tests/testthat/test-glmm.R | Sets fancy quotes option for tests |
| tests/testthat/test-auxvars.R | New test file for auxiliary variables functionality |
| R/helpfunctions_formulas_general.R | Refactors formula parsing functions; all_vars() has potential bugs with NULL/non-character handling |
| R/helpfunctions_formulas.R | Refactors extract_outcomes_list() and remove_grouping(); adds new helper functions |
| R/helpfunctions_checks.R | Refactors data validation functions; contains inconsistent assignment operators (= vs <-) |
| R/helpfunctions_vcov.R | Updates get_nranef() signature to remove idvar parameter |
| R/helpfunctions_ranefs.R | Adds new rd_terms_by_grouping() function |
| R/helpfunctions.R | Extracts grouping validation into separate documented functions |
| R/summary.JointAI.R | Fixes thin parameter handling and sample size calculation |
| R/predict.R | Updates function calls to use renamed functions |
| R/model_imp.R | Updates check_data() call to include warn parameter |
| R/helpfunctions_divide_matrices.R | Updates all function calls to use renamed functions |
| R/helpfunctions_JAGS.R | Updates parallel worker count access method |
| R/helpfunctions_melt.R | Replaces lambda syntax with traditional function syntax for compatibility |
| R/get_refs.R | Updates all_vars() usage |
| R/get_modeltypes.R | Updates function calls and improves all_vars() usage |
| R/divide_matrices.R | Updates function calls to use extract_grouping() |
| man/*.Rd | Updates and adds documentation for refactored functions |
| NEWS.md | Documents bug fix and improvements |
| DESCRIPTION | Updates version number to development version |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
R/helpfunctions_checks.R
Outdated
Comment on lines
+324
to
+330
| class_change = Filter(isTRUE, class_change) | ||
|
|
||
| level_change <- mapply(function(x1, x2) !isTRUE(all.equal(x1, x2)), | ||
| x1 = lapply(data1, levels), | ||
| x2 = lapply(data2, levels) | ||
| ) | ||
| level_change = Filter(isTRUE, level_change) |
There was a problem hiding this comment.
Inconsistent use of assignment operator: Use <- instead of = for assignment to match R coding conventions and maintain consistency with the rest of the codebase.
Suggested change
| class_change = Filter(isTRUE, class_change) | |
| level_change <- mapply(function(x1, x2) !isTRUE(all.equal(x1, x2)), | |
| x1 = lapply(data1, levels), | |
| x2 = lapply(data2, levels) | |
| ) | |
| level_change = Filter(isTRUE, level_change) | |
| class_change <- Filter(isTRUE, class_change) | |
| level_change <- mapply(function(x1, x2) !isTRUE(all.equal(x1, x2)), | |
| x1 = lapply(data1, levels), | |
| x2 = lapply(data2, levels) | |
| ) | |
| level_change <- Filter(isTRUE, level_change) |
Contributor
Contributor
Co-authored-by: NErler <10401637+NErler@users.noreply.github.com>
Co-authored-by: NErler <10401637+NErler@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Fix grammatical error in test description
Fix typo in test description: extact_lhs → extract_lhs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Started to re-write some functions to improve readability, and added documentation to those (internal) function. In the process, also some bugs were fixed (for functions in model formulas and the check for whether sampling will be run in parallel).