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

funs() function, deprecated in {dplyr} 0.8.0, has been modified so that it is not used and warning messages are not displayed. #27

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

indenkun
Copy link

@indenkun indenkun commented Jul 4, 2022

If I specify anything other than identity for the trans argument in coefplot(), I get the following warning message.

library(coefplot)

model2 <- glm(price > 10000 ~ carat + cut*color, data=diamonds, family=binomial(link="logit"))
coefplot(model2, trans=invlogit)

    ## Warning: `funs()` was deprecated in dplyr 0.8.0.
    ## Please use a list of either functions or lambdas: 
    ## 
    ##   # Simple named list: 
    ##   list(mean = mean, median = median)
    ## 
    ##   # Auto named with `tibble::lst()`: 
    ##   tibble::lst(mean, median)
    ## 
    ##   # Using lambdas
    ##   list(~ mean(., trim = .2), ~ median(., na.rm = TRUE))
    ## This warning is displayed once every 8 hours.
    ## Call `lifecycle::last_lifecycle_warnings()` to see where this warning was generated.

# Figure omitted.

As you can see in the message, this is due to the use of funs(), which has been deprecated since {dplyr} 0.8.0.
Currently there is no problem with the result, but it is hoped that this will be corrected in the future, as deprecated functions may be removed in the future.

The code I pulled is expected to work as before without any warning messages.
If you do not like it, please discard it.

…o that it is not used and warning messages are not displayed.
Copy link
Owner

@jaredlander jaredlander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. I think mutate_at() has been superseded, so perhaps mutate(across(...)) will be better?

@indenkun
Copy link
Author

indenkun commented Jul 5, 2022

I think it is better to replace mutate_at() with mutate(acrros(...)) .

I will write and Pull Request if it is ok with the code replaced with mutate(across(...)).

@jaredlander
Copy link
Owner

I would love that, thanks

@indenkun
Copy link
Author

indenkun commented Jul 6, 2022

Thank you for your reply.

Replaced mutate_at() with mutate(across(...)).
It should return the same results as before.

@indenkun
Copy link
Author

indenkun commented Jul 8, 2022

Speaking of which, because across() is a {dplyr} 1.0.0 or later function, can I rewrite the version specification in DESCRIPTION?

@jaredlander
Copy link
Owner

Oh, that brings up a good point. I have very mixed feelings about forcing a recent version of {dplyr}, or any package, on users. That can be very disruptive. I have to think if that's the best way forward, or if we should stick with mutate_at().

I know you put a lot of effort into rewriting this PR so I don't want that to go to waste, but I think it's important to be conscientious about this.

@indenkun
Copy link
Author

indenkun commented Jul 8, 2022

Oh, I thought I rewrote buildModelCI.default() (9cf1590), but did you not receive it?
Was it incomplete?
Did I do something wrong in my use of GitHub?

Or maybe you also expect the rewrite of mutate_at() in the other part (extract.coef.xgb.Booster())?
Then do you also expect to rewrite filter_at and other *_at types?

The rewriting doesn't seem to be that hard, but I think it will take some time to test because some of the expected results are not clear.

Also, is it correct that DESCRIPTION should not be rewritten?

I am a non-native speaker of English, so I apologize if I have misinterpreted your meaning.

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.

2 participants