search

sinhrks / ggfortify

Define fortify and autoplot functions to allow ggplot2 to handle some popular R packages.
Other
525 stars 65 forks source link

Updating many files to address dplyr XXX_() variants being deprecated #208

Closed dereksonderegger closed 3 years ago

dereksonderegger commented 3 years ago

I've updated many of the files. The other pull request you received had a problem where 

select_cols <- c('Petal.Length','Sepal.Length')
iris %>%
#  select(select_cols)   #  Generates a warning...
    select(all_of(select_cols))  # is recommended

the first version generates a warning and tells us to use the all_of() command instead.

Also, there was a do() function call plotlib.R that was throwing a warning about using progress bars, but I can't seem to recreate it now, so I decided not to change that piece. The dplyr::do() command is superseded and not recommended and I thought about swapping it out with 

dplyr::summarize(across(grDevices::chull()))

but decided against it.

When I run the package tests, there are still some unresolved problems, but I think they were there when I first forked the package. So double checking the tests would be highly recommended.

terrytangyuan commented 3 years ago

The changes look great!

Seems like we are seeing the following failure:

── 1. Failure: autoplot ts works for multivariate timeseries (@test-ts.R#169)  ─

ld$ymin not equal to c(0, 0, 0, 0, 1, 2, 3, 4).

6/8 mismatches (average diff: 2.17)

[3] 1 - 0 ==  1

[4] 2 - 0 ==  2

[5] 0 - 1 == -1

[6] 0 - 2 == -2

[7] 6 - 3 ==  3

[8] 8 - 4 ==  4

══ testthat results  ═══════════════════════════════════════════════════════════

[ OK: 894 | SKIPPED: 44 | WARNINGS: 1 | FAILED: 1 ]

1. Failure: autoplot ts works for multivariate timeseries (@test-ts.R#169)

Is this what you are seeing locally when running the tests?

terrytangyuan commented 3 years ago

You can find a list of past CI builds for PRs here. I do not see this test failing for the last PR so it might have been introduced from the changes here.

dereksonderegger commented 3 years ago

I do see that test failure but wasn't sure if it was a previous failure. I'll investigate and fix it.

dereksonderegger commented 3 years ago

The surprising error was that the sort order changed going from gather_() to pivot_longer() and for some reason later on, it matters.

codecov[bot] commented 3 years ago

Codecov Report

Merging #208 into master will increase coverage by 0.00%. The diff coverage is 59.37%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #208   +/-   ##
=======================================
  Coverage   62.23%   62.23%           
=======================================
  Files          22       22           
  Lines        1774     1777    +3     
=======================================
+ Hits         1104     1106    +2     
- Misses        670      671    +1
Impacted Files Coverage Δ
R/fortify_basis.R 0.00% <0.00%> (ø)
R/fortify_changepoint.R 0.00% <0.00%> (ø)
R/fortify_cluster.R 0.00% <0.00%> (ø)
R/fortify_forecast.R 0.00% <0.00%> (ø)
R/fortify_stats_lm.R 0.00% <0.00%> (ø)
R/base_fortify_ts.R 68.58% <100.00%> (+0.16%) :arrow_up:
R/fortify_spatial.R 96.66% <100.00%> (ø)
R/fortify_stats.R 95.91% <100.00%> (ø)
R/fortify_surv.R 94.11% <100.00%> (ø)
R/fortify_vars.R 100.00% <100.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update e9eb915...9d660c9. Read the comment docs.