patperry / r-mbest

Moment-Based Estimation for Hierarchical Models (R Package)
Apache License 2.0
24 stars 6 forks source link

potential upstream change in formula processing #12

Open bbolker opened 3 months ago

bbolker commented 3 months ago

I am planning to switch lme4 over to using the reformulas package for formula processing. I tried to keep the results completely identical, but there is one slight change that's breaking the tests in mbest and I wonder if it's important to restore the original behaviour. Specifically, lme4::findbars() adds parentheses to the expansion of a nested grouping variable, while reformulas::findbars() doesn't (see example below). In principle this shouldn't matter (the : operator in a formula context should be associative), but if the parentheses are semantically meaningful in your context I'm open to doing some more work on reformulas to try restore the previous behaviour ... otherwise, a two-character change in your tests should fix everything.

f <- y ~ 1 + (1|g1/g2/g3)
> lme4::findbars(f)
[[1]]
1 | g3:(g2:g1)

[[2]]
1 | g2:g1

[[3]]
1 | g1

> reformulas::findbars(f)
[[1]]
1 | g3:g2:g1

[[2]]
1 | g2:g1

[[3]]
1 | g1
patperry commented 3 months ago

Thanks for the heads up about this. I'm didn't write the impacted code or tests, so I'm not clear on the impact here. @kschmaus do you have any insight?

kschmaus commented 3 months ago

This is a blast from the past! I don't think the parenthesis should be syntactically relevant for mbest, so I'm fairly sure g3:g2:g1 should be just fine.

bbolker commented 3 months ago

OK. It will be a little while until this becomes relevant. I will push a new version of reformulas to CRAN shortly, then sometime after that a version of lme4 that uses/re-exports the machinery from reformulas. If in the meantime you want to change this test and switch your package from depending on lme4 to depending on reformulas, you won't have to wait for me (the current CRAN version of reformulas should work fine).