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

find_formula: remove double "+" symbols with lmBF #566

Merged
merged 5 commits into from
Oct 10, 2022

Conversation

etiennebacher
Copy link
Member

@etiennebacher etiennebacher commented May 17, 2022

I noticed that using find_formula with lmBF() keeps two consecutive + in the conditional part of the formula. I suppose this is a bug so this PR removes the duplicate + (but I never had an issue with sth like that, it just looked like a bug).

Before:

library(BayesFactor)
library(insight)

mtcars$cyl <- factor(mtcars$cyl)
mtcars$gear <- factor(mtcars$gear)

model <- lmBF(mpg ~ cyl + gear + cyl:gear, mtcars, 
              progress = FALSE, whichRandom = c("gear", "cyl:gear"))

find_formula(model)
#> $conditional
#> mpg ~ cyl + +cyl:gear
#> <environment: 0x000001fe2b14b560>
#> 
#> $random
#> ~gear
#> <environment: 0x000001fe2b14b560>
#> 
#> attr(,"class")
#> [1] "insight_formula" "list"

Created on 2022-05-17 by the reprex package (v2.0.1)

After:

library(BayesFactor)
library(insight)

mtcars$cyl <- factor(mtcars$cyl)
mtcars$gear <- factor(mtcars$gear)

model <- lmBF(mpg ~ cyl + gear + cyl:gear, mtcars, 
              progress = FALSE, whichRandom = c("gear", "cyl:gear"))

find_formula(model)
#> $conditional
#> mpg ~ cyl + cyl:gear
#> <environment: 0x0000020ec4097bd8>
#> 
#> $random
#> ~gear
#> <environment: 0x0000020ec4097bd8>
#> 
#> attr(,"class")
#> [1] "insight_formula" "list"

Created on 2022-05-17 by the reprex package (v2.0.1)

@bwiernik
Copy link
Contributor

Shouldn't cyl:gear be in the random section too?

@etiennebacher
Copy link
Member Author

@bwiernik I don't know, I never use this kind of models, I just stumbled upon this while looking at easystats/bayestestR#505 because it seemed to be an issue I could try to solve.

I just assumed that the output of find_formula was good except for this duplicated +. If it's not the case, I will just close this PR

@bwiernik
Copy link
Contributor

@strengejacke based on the which_random argument, shouldn't the product term be in random too?

@strengejacke
Copy link
Member

I don't use the BayesFactor package, I think this is something @mattansb implemented?

tests/testthat/test-BayesFactorBF.R Outdated Show resolved Hide resolved
@mattansb
Copy link
Member

@bwiernik yeah, something is off there. I also think I implanted this support for BayesFactor.
I will have a chance to take a look later this week / earlier next week.

Thanks @etiennebacher !

@mattansb
Copy link
Member

This seems like a problem with BayesFactor.

  1. Data types (fixed / continuous / random) are per predictor, not per term.
  2. It seems like adding "cyl:gear" as random has no effect on the model?
library(BayesFactor)

mtcars$cyl <- factor(mtcars$cyl)
mtcars$gear <- factor(mtcars$gear)

model1 <- lmBF(mpg ~ cyl + gear + cyl:gear, mtcars, 
              progress = FALSE, whichRandom = c("gear", "cyl:gear"))

model2 <- lmBF(mpg ~ cyl + gear + cyl:gear, mtcars, 
               progress = FALSE, whichRandom = c("gear"))

model2 / model1
#> Bayes factor analysis
#> --------------
#> [1] cyl + gear + cyl:gear : 1 ±0%
#> 
#> Against denominator:
#>   mpg ~ cyl + gear + cyl:gear 
#> ---
#> Bayes factor type: BFlinearModel, JZS

Created on 2022-05-19 by the reprex package (v2.0.1)

@bwiernik
Copy link
Contributor

Okay it looks like the output is correct given the way that BF is implemented

@strengejacke
Copy link
Member

What's the state of this PR?

@codecov-commenter
Copy link

Codecov Report

Merging #566 (3b6cf10) into main (365a898) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #566   +/-   ##
=======================================
  Coverage   55.10%   55.10%           
=======================================
  Files         124      124           
  Lines       14341    14343    +2     
=======================================
+ Hits         7902     7904    +2     
  Misses       6439     6439           
Impacted Files Coverage Δ
R/find_formula.R 64.56% <100.00%> (+0.09%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@bwiernik bwiernik merged commit c68af2c into easystats:main Oct 10, 2022
@etiennebacher etiennebacher deleted the find-formula-lmBF branch October 10, 2022 07:29
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.

6 participants