Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix issue 658 by reproducing formula function #659

Merged

Conversation

iago-pssjd
Copy link

Close issue #658. The idea, as told there, is to do fm <- formula(x$terms) instead of fm <- eval(x$call$fixed) , but this would need to include importFrom("stats", formula) in the NAMESPACE, and since there is no importFrom there, I assumed you prefer not use them. Therefore, instead of using formula(....), I wrote the code of stats:::formula.terms there.

@vincentarelbundock
Copy link
Contributor

Thanks for opening a pull request!

You don't need to use importFrom or to re-write the code yourself. Instead, you can simply call the function from its namespace:

fm <- stats::formula(x$terms)

Are we sure that the two versions will always give the same results? What if there are variables in both the random and fixed components/formulas? Do both codes give the same results in a model estimated outside a lapply?

I'm not sure what the original intent of the code was in that respect, so it would be important to make sure nothing changes in different models.

@iago-pssjd
Copy link
Author

iago-pssjd commented Oct 6, 2022

@vincentarelbundock thanks for the point of using stats::formula. I believe this is more a stylistic which you, the package developers, know better than me your preferences.

Are we sure that the two versions will always give the same results? What if there are variables in both the random and fixed components/formulas? Do both codes give the same results in a model estimated outside a lapply?

I'm not sure what the original intent of the code was in that respect, so it would be important to make sure nothing changes in different models.

It seems so, at least with the original formulas. I just have built, installed and checked --as-cran the package without any complaints. It lacks a revdep check, so it does not changes other packages behaviours, but I am not sure how and do not have enough time to do it.

Update: I forgot to say that I had looked for the details of the terms element but it seems to be undocumented (?lmeObject)

@iago-pssjd
Copy link
Author

You don't need to use importFrom or to re-write the code yourself. Instead, you can simply call the function from its namespace

I updated the code using now stats::formula.

@strengejacke
Copy link
Member

Do we have some example code where the former code failed, and your PR works? We could then also use this demonstration to add some tests.

@iago-pssjd
Copy link
Author

Yes! Issue #658:

library(insight)
library(nlme)
models <-lapply(c("", " + Sex"), \(.x) lme(as.formula(paste0("distance  ~ age", .x)), random = ~ 1, data = Orthodont))
lapply(models, find_formula)

Or just find_formula(models[[1]])

@vincentarelbundock vincentarelbundock merged commit 3cb562b into easystats:main Oct 7, 2022
@vincentarelbundock
Copy link
Contributor

Thanks a lot for the PR!

I merged it and added a test here: #660

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants