Spring cleaning and refactoring #242
Conversation
…d method definitions
…ve variable handling
…r handling default priors
- split set_default_priors into smaller functions - add lhs_vars and unnest_list functions - additional tests
…et_size predictors as separate functions; improve code readability
venpopov
added
PR - minor
|
@GidonFrischkorn I need to resolve some conflicts due to merging the M3 into develop, but after that it should be good to look at. |
@GidonFrischkorn this is good for review now |
| formula <- assign_nl(formula) | ||
| assign_constants(formula) | ||
| out <- list(...) | ||
| is_form_or_num <- function(x) is_formula(x) || (is.numeric(x) && length(x) == 1) |
There was a problem hiding this comment.
Is there a specific reason for defining this function within the bmmformula function? Would it not be better to define it as an internal function outside of the bmmformula.
I assume there is a reason in that this formula is only used inside this formula.
There was a problem hiding this comment.
That's exactly the reason. It is a strange condition that is unlikely to occur in many places. In principle we don't need a named function at all - we could just use an anonymous function directly in the sapply call right below it. This is just a new trick I learned, where instead of writing comments that explain what the code does, which might become outdated, is to write the code in a way that says what it does directly. In this particular case instead of reading this:
stopif(
!all(sapply(out, function(x) is_formula(x) || (is.numeric(x) && length(x) == 1))),
"Arguments must be formulas or numeric assignments."
)and then trying to deparse what the logic is, the function name tells you what condition it checks for. But by keeping the function definition right there, you can still see the code without having to jump to another part of the code-base. If it turns out that we need it anywhere else, we can move it outside.
In this case it's not a huge difference, but it's a nice concept I learned about and I'm trying out in the code in a few places
Summary
I hadn't looked at the code in months. While I was reviewing #237, I saw a lot of things in the existing code that I wanted to refactor with some things I've learned over the last half-year. I haven't changed any functionality, just streamlined the code and broke up some functions that were too big and convoluted (mostly breaking up deeply nested if/else and for loops). I also got rid of the dplyr and tidyr dependencies, which we used in only a couple of places. I've added a bunch of extra tests and removed comments which were outdated and did not reflect what the code was actually doing. Changed a bunch of variable names so that the intent is clear without the need for comments.
All the tests pass and there is no substantial new functionality. If you would like to run a couple of models you already have to make sure nothing breaks on your end.
I will continue now the review of #237, but the changes here would need to be merged there as well.
Closes #238, closes #239, closes #240
Tests
[X] Confirm that all tests passed
[X] Confirm that devtools::check() produces no errors
Release notes