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

Implements tidy.Kendall + adds info in DESCRIP, NAMESPACE, etc.. #343

Merged
merged 1 commit into from Jun 11, 2018

Conversation

Projects
None yet
3 participants
@cimentadaj
Contributor

cimentadaj commented Jun 10, 2018

Following #285, I've added tidy.Kendall which creates a tidy method for all Kendall class objects. These come from the Kendall package and only have 3 functions: Kendall, MannKendall and SeasonalMannKendall. They all return the same information so the tidy method works for the three functions. The number of rows and columns are always the same and there are not optional arguments on the three functions.

Here's a brief example of what the tidy.Kendall will return

library(Kendall)
library(broom)

### For Kendall ####
A <- c(2.5, 2.5, 2.5, 2.5, 5, 6.5, 6.5, 10, 10, 10, 10, 10, 14, 14, 14, 16, 
  17)
B <- c(1, 1, 1, 1, 2, 1, 1, 2, 1, 1, 1, 1, 1, 1, 2, 2, 2)
f_res <- Kendall(A, B)

tidy(f_res)
#> # A tibble: 1 x 5
#>     tau p.value kendall_score denominator var_kendall_score
#>   <dbl>   <dbl>         <dbl>       <dbl>             <dbl>
#> 1 0.408  0.0754            34        83.4              345.

### For MannKendall ####
s_res <- MannKendall(B)

tidy(s_res)
#> # A tibble: 1 x 5
#>     tau p.value kendall_score denominator var_kendall_score
#>   <dbl>   <dbl>         <dbl>       <dbl>             <dbl>
#> 1 0.354   0.102            32        90.3               360

### For SeasonalMannKendall ####
t_res <- SeasonalMannKendall(ts(A))

tidy(t_res)
#> # A tibble: 1 x 5
#>     tau     p.value kendall_score denominator var_kendall_score
#>   <dbl>       <dbl>         <dbl>       <dbl>             <dbl>
#> 1 0.924 0.000000935           116        126.              559.

Everything I've done so far:

  • devtools::check() is not passing but there are no warnings from tidy.Kendall.
  • goodpractice::gp() passes for tidy.Kendall
  • devtools::spell_check() only returns the word kendall which is a false positive.
  • I've reviewed the tidyverse documentation guidelines and fixed everything accordingly in the docs of tidy.Kendall
  • I've updated NEWS.md to include tidy.Kendall.
  • I've added tests using check_tidy in test-kendall.R
  • I've added the Kendall package as Suggests
  • I've added myself as ctb

@cimentadaj cimentadaj referenced this pull request Jun 10, 2018

Closed

Kendall #285

@rbloehm

This comment has been minimized.

rbloehm commented Jun 10, 2018

Hi cimentada, there seems to be an issue with the line endings (should probably be LF) or the encoding (should be UTF-8) of the files in your commit.
See for example DESCRIPTION, NAMESPACE, NEWS.md. You should only see changes in the 1-2 lines you touched, but somehow every single line changed.
broom.Rproj actually hasn't changed at all, but is still shown as changed in every single row.

@cimentadaj

This comment has been minimized.

Contributor

cimentadaj commented Jun 10, 2018

@rbloehm You're completely right. I have this behaviour ever since I started using Git and Bash in Windows 10. I'm not sure how to fix it at this point with out reverting everything back and re implementing the changes.

@rbloehm

This comment has been minimized.

rbloehm commented Jun 10, 2018

Sorry about that, I hope you will find some guidance here:
https://help.github.com/articles/dealing-with-line-endings/#platform-windows

Especially make sure that
git config --global core.autocrlf true
is set globally on your Windows machine.

@alexpghayes

This comment has been minimized.

Collaborator

alexpghayes commented Jun 10, 2018

This looks really good! Thanks for taking the time to make a PR!

I have a suspicion that at least one of the columns in the output should be called statistic for consistency, but won't be able to check until the work week. Besides that, let's figure out how to get the line endings fixed and then get this thing merged!

@cimentadaj cimentadaj force-pushed the cimentadaj:tidy.Kendall branch 5 times, most recently from da83477 to 834be8b Jun 11, 2018

@cimentadaj cimentadaj force-pushed the cimentadaj:tidy.Kendall branch from 834be8b to 850b2bd Jun 11, 2018

@cimentadaj

This comment has been minimized.

Contributor

cimentadaj commented Jun 11, 2018

@alexpghayes @rbloehm I fixed the problem with line endings and changed the column name of tau to statistic for consistency. Let me know if that's alright!

@alexpghayes alexpghayes merged commit ccc6878 into tidymodels:master Jun 11, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@alexpghayes

This comment has been minimized.

Collaborator

alexpghayes commented Jun 11, 2018

Looks great, thank you!

@cimentadaj cimentadaj deleted the cimentadaj:tidy.Kendall branch Jun 11, 2018

@rbloehm

This comment has been minimized.

rbloehm commented Jun 11, 2018

@alexpghayes: Was still committed and merged into master with CRLF in the new files. Run this in the latest master of tidyverse/broom:
git grep -I --files-with-matches --perl-regexp '\r' HEAD
image
Still a nice PR though, thanks @cimentadaj

@alexpghayes

This comment has been minimized.

Collaborator

alexpghayes commented Jun 11, 2018

Thanks!

@alexpghayes alexpghayes referenced this pull request Jun 18, 2018

Closed

Clean up line endings #385

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment