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

Overwrite formula.tools::as.character.formula() #54

Closed
wants to merge 1 commit into from
Closed

Overwrite formula.tools::as.character.formula() #54

wants to merge 1 commit into from

Conversation

etiennebacher
Copy link

Hello,

I ran into the same issue as #49 where loading logistf creates errors in MCMCglmm because of the dependency on formula.tools, and more specifically the fact that the latter package exports as.character.formula(). It would be better if this issue was fixed directly in formula.tools but unfortunately it seems quite abandoned, which is why I open a PR here.

The objective of this PR is just to overwrite formula.tools::as.character.formula() so that it goes back to the R default. One important thing to note is that I don't know if this change breaks something in the current code because there is no tests suite for me to check if it has side effects.

Here's an example:

######## Before loading logistf, it works fine

library(MCMCglmm)
#> Loading required package: Matrix
#> Loading required package: coda
#> Loading required package: ape

data(PlodiaPO)
invisible(capture.output(
    x <- MCMCglmm(PO ~ plate, data = PlodiaPO)
))

######## After loading logistf, it breaks

library(logistf)
packageVersion("logistf")
#> [1] '1.24.5'

# Same as above
data(PlodiaPO)
invisible(capture.output(
    x <- MCMCglmm(PO ~ plate, data = PlodiaPO)
))
#> Error in terms.formula(formula, data = data): invalid term in model formula

And here's the behavior after this fix:

library(MCMCglmm)
#> Loading required package: Matrix
#> Loading required package: coda
#> Loading required package: ape

data(PlodiaPO)
invisible(capture.output(
    x <- MCMCglmm(PO ~ plate, data = PlodiaPO)
))

library(logistf)
#> Registered S3 method overwritten by 'logistf':
#>   method               from         
#>   as.character.formula formula.tools
packageVersion("logistf")
#> [1] '1.24.5'

# Same as above
data(PlodiaPO)
invisible(capture.output(
    x <- MCMCglmm(PO ~ plate, data = PlodiaPO)
))

This is not ideal, the best would probably be to drop the formula.tools dependency (as you point out in #49).

Thank you

@gregorsteiner
Copy link
Collaborator

Hi Etienne,

thanks for your contribution. I just ran a quick check. Unfortunately, this change does cause errors elsewhere. We will work on trying to find a better solution. Ideally, we can drop the formula.tools dependency with the next update.

@etiennebacher
Copy link
Author

Great, looking forward to the next update then, thank you.

@etiennebacher etiennebacher deleted the fix-as-character branch May 8, 2023 06:33
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.

None yet

2 participants