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

New CRAN release #649

Merged
merged 21 commits into from
Jul 3, 2024
Merged

New CRAN release #649

merged 21 commits into from
Jul 3, 2024

Conversation

mattansb
Copy link
Member

deal with #648

deal with #648
@mattansb
Copy link
Member Author

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

@IndrajeetPatil
Copy link
Member

IndrajeetPatil commented Jun 23, 2024

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

@strengejacke
Copy link
Member

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

@strengejacke
Copy link
Member

strengejacke commented Jun 23, 2024

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

@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
Copy link
Member Author

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

@strengejacke
Copy link
Member

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

@IndrajeetPatil
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

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

Anyway, submitted.

@mattansb
Copy link
Member Author

mattansb commented Jul 3, 2024

Thanks, on its way to CRAN.

@mattansb mattansb merged commit 5adb681 into main Jul 3, 2024
21 of 22 checks passed
@mattansb mattansb deleted the CRAN-0.8.9 branch July 3, 2024 08:36
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