easystats / effectsize

:dragon: Compute and work with indices of effect size and standardized parameters
https://easystats.github.io/effectsize/
Other
338 stars 24 forks source link

New CRAN release #649

Closed mattansb closed 4 months ago

mattansb commented 4 months ago

deal with #648

mattansb commented 4 months ago

@strengejacke @IndrajeetPatil Any idea why some tests are still failing when effectsize no longer uses the effectsize_type argument anywhere?

IndrajeetPatil commented 4 months ago

You need to use parameters 0.22.0, which has renamed this parameter. Bump the version in DESCRIPTION and hopefully it goes away.

strengejacke commented 4 months ago

It's indeed strange, we have this warning only since parameters 0.22.0?

strengejacke commented 4 months ago

You're calling, e.g. from .anova_es.aov():

params <- parameters::model_parameters(model, verbose = verbose, effects = "fixed", es_type = NULL)

the .aov method from model_parameters has no effects argument, and this partially matches with the deprecated, but not yet removed(!) argument effectsize_type. The only way to fix this is to remove the effects argument in your call in .anova_es.aov() (and in the other functions)

IndrajeetPatil commented 4 months ago

Oh boy. Okay, I have changed some options to make sure that we always get warnings if we are using partial matching anywhere.

You should see new warnings from now on about this issue.

strengejacke commented 4 months ago

Our goal was to remove that partial matching by renaming that argument in parameters, although there were no warnings, so Mattan can use both effects and es_type in his effectsize package - and now we actually made things worse (at least temporary...) 🙄

strengejacke commented 4 months ago

@mattansb Do we need to set effects = "fixed" in the anova methods at all? That argument doesn't exist in parameters::model_parameters() for anova objects.

mattansb commented 4 months ago

We do :/ some non-ANOVA objects get passed down there... Is there a verbose = FALSE option to suppress that partial matching warning?

strengejacke commented 4 months ago

No, there's not. 😞 Maybe we wrap the call into suppressWarnings+)?

IndrajeetPatil commented 4 months ago

You can also change the options generating warnings about partial matching using on.exit() for the scope of the function where this warning originates.

mattansb commented 4 months ago

But I want to capture other warnings...

I think I'll have to use:

foo <- function(x) {
  if (x == 1) {
    warning("something something effectsize_type...")
  } else {
    warning("some other warning...!")
  }
  x + 1
}

foo(1)
#> Warning in foo(1): something something effectsize_type...
#> [1] 2

foo(2)
#> Warning in foo(2): some other warning...!
#> [1] 3
withCallingHandlers({
  foo(1)
}, warning = function(w) {
  if (grepl("effectsize_type...", conditionMessage(w))) {
    invokeRestart("muffleWarning")
  }
})
#> [1] 2
withCallingHandlers({
  foo(2)
}, warning = function(w) {
  if (grepl("effectsize_type...", conditionMessage(w))) {
    invokeRestart("muffleWarning")
  }
})
#> Warning in foo(2): some other warning...!
#> [1] 3

Created on 2024-06-24 with reprex v2.1.0

mattansb commented 4 months ago

Okay, @strengejacke 's fix seems to work (and all this lints! Wow!) - but I remember that argument was there for a reason. I'm sure it will come up eventually (:

Thanks!

strengejacke commented 4 months ago

I'm sure it will come up eventually (:

I'd say we quickly remove the deprecation warning in parameters, and add back the effects argument then. Maybe we add a "TODO" as comment to the code?

strengejacke commented 4 months ago

but I remember that argument was there for a reason

I think I also read an issue about this, but cannot quite remember when and where.

strengejacke commented 4 months ago

Was it related to the report package? Or because we could have parameter effect sizes for mixed models, and therefore needed to set effects = "fixed"?

mattansb commented 4 months ago

No idea 🤷‍♂️ We test mixed models, so it's not it.

Anyway, submitted.

mattansb commented 4 months ago

Thanks, on its way to CRAN.