Submission tsbox: Class-Agnostic Time Series #464
Comments
|
Thanks for submitting to rOpenSci, our editors and @ropensci-review-bot will reply soon. Type |
|
Editor check started |
|
Oops, something went wrong with our automatic package checks. Our developers have been notified and package checks will appear here as soon as we've resolved the issue. Sorry for any inconvenience. |
|
@christophsax Sorry about the mess up here - the checks server is currently running R4.0.3, so doesn't recognise native pipes which you use in some test code. Will upgrade asap and deliver your checks. |
|
But I did't intend to use native pipes, that was an accident. I will remove them right now. |
|
I did so now. Perhaps rerunning is sufficient. |
Checks for tsbox (v0.3.1.9001)git hash: aad57cea
Important: All failing checks above must be addressed prior to proceeding Package License: GPL-3 1. rOpenSci Statistical Standards (
|
| measure | value | percentile | noteworthy |
|---|---|---|---|
| files_R | 53 | 96.0 | |
| files_vignettes | 3 | 90.8 | |
| files_tests | 44 | 98.7 | |
| loc_R | 2649 | 88.3 | |
| loc_vignettes | 344 | 80.6 | |
| loc_tests | 1951 | 92.1 | |
| num_vignettes | 3 | 93.1 | |
| n_fns_r | 340 | 93.4 | |
| n_fns_r_exported | 63 | 90.4 | |
| n_fns_r_not_exported | 277 | 93.8 | |
| n_fns_per_file_r | 4 | 51.4 | |
| num_params_per_fn | 1 | 1.1 | TRUE |
| loc_per_fn_r | 8 | 29.0 | |
| loc_per_fn_r_exp | 7 | 14.2 | |
| loc_per_fn_r_not_exp | 9 | 41.9 | |
| rel_whitespace_R | 25 | 91.6 | |
| rel_whitespace_vignettes | 32 | 87.8 | |
| rel_whitespace_tests | 28 | 98.0 | TRUE |
| doclines_per_fn_exp | 41 | 51.1 | |
| doclines_per_fn_not_exp | 0 | 0.0 | TRUE |
| fn_call_network_size | 475 | 94.6 |
2a. Network visualisation
Interactive network visualisation of calls between objects in package can be viewed by clicking here
3. goodpractice and other checks
Details of goodpractice and other checks (click to open)
3a. Continuous Integration Badges
GitHub Workflow Results
| name | conclusion | sha | date |
|---|---|---|---|
| R-CMD-check | success | aad57c | 2021-09-18 |
3b. goodpractice results
R CMD check with rcmdcheck
R CMD check generated the following error:
- checking tests ...
Running ‘spelling.R’
Running ‘testthat.R’
ERROR
Running the tests in ‘tests/testthat.R’ failed.
Last 13 lines of output:
█- ├─fl[i]
- │ └─tsbox::ts_apply(x, ff, ...)
- │ └─tsbox:::ts_apply_dts(ts_dts(x), fun, ...)
- │ ├─x[, fun(.SD, ...), by = eval(.by)]
- │ └─data.table:::
[.data.table(x, , fun(.SD, ...), by = eval(.by)) - └─tsbox:::fun(.SD, ...)
- └─(function(x, ...) seasonal::final(seasonal::seas(x, ...)))(...)
-
├─seasonal::final(seasonal::seas(x, ...)) -
└─seasonal::seas(x, ...) -
└─seasonal::checkX13(fail = TRUE, fullcheck = FALSE, htmlcheck = FALSE)
[ FAIL 1 | WARN 8 | SKIP 17 | PASS 644 ]
Error: Test failures
Execution halted
R CMD check generated the following test_fail:
-
library(testthat)
Warning message:
package 'testthat' was built under R version 4.1.0
library(tsbox)
test_check("tsbox")
Starting 2 test processes
══ Skipped tests ═══════════════════════════════════════════════════════════════
• On CRAN (17)
══ Failed tests ════════════════════════════════════════════════════════════════
── Error (test-units.R:41:5): tsbox works with units ───────────────────────────
Error: Process terminated
Backtrace:
█
- ├─fl[i]
- │ └─tsbox::ts_apply(x, ff, ...)
- │ └─tsbox:::ts_apply_dts(ts_dts(x), fun, ...)
- │ ├─x[, fun(.SD, ...), by = eval(.by)]
- │ └─data.table:::
[.data.table(x, , fun(.SD, ...), by = eval(.by)) - └─tsbox:::fun(.SD, ...)
- └─(function(x, ...) seasonal::final(seasonal::seas(x, ...)))(...)
-
├─seasonal::final(seasonal::seas(x, ...)) -
└─seasonal::seas(x, ...) -
└─seasonal::checkX13(fail = TRUE, fullcheck = FALSE, htmlcheck = FALSE)
[ FAIL 1 | WARN 8 | SKIP 17 | PASS 644 ]
Error: Test failures
Execution halted
R CMD check generated the following check_fail:
- rcmdcheck_tests_pass
Test coverage with covr
ERROR: Test Coverage Failed
Cyclocomplexity with cyclocomp
The following functions have cyclocomplexity >= 15:
| function | cyclocomplexity |
|---|---|
| ts_span | 34 |
| time_shift | 21 |
| ts_plot | 21 |
| copy_class | 19 |
Static code analyses with lintr
lintr found the following 178 potential issues:
| message | number of times |
|---|---|
| Lines should not be more than 80 characters. | 178 |
Package Versions
| package | version |
|---|---|
| pkgstats | 0.0.0.311 |
| pkgcheck | 0.0.1.486 |
| srr | 0.0.1.107 |
Editor-in-Chief Instructions:
Processing may not proceed until the items marked with
R CMD checkThere seems to be an issue with the package seasonal on your system. The package is special since it uses x13binary which downloads pre-built binaries from CRAN. Usually this works fine and on most CRAN platforms. Is there anything special about your system? There was an issue for Mac M1. If you want to check, this should works: library(seasonal)
seas(AirPassengers)
checkX13()But I don't want you to drag into this. I can also remove the few tests that are using seasonal, which should then resolve the issue? CoveragePerhaps this is just because of the failing R CMD check? If not, is there a way to see what ist the measured coverage? I had 81.2% on my system, which passed the threshold (see below). Report on my sideIf I run #### Coverage
✔ Package uses 'roxygen2'.
✔ Package has a 'contributing.md' file.
✔ Package has a 'CITATION' file.
✔ Package has a 'codemeta.json' file.
✔ All functions have examples.
✔ Package has at least one HTML vignette
✔ Package 'DESCRIPTION' has a URL field.
✔ Package 'DESCRIPTION' has a BugReports field.
✔ Package is already on CRAN.
✔ Package has continuous integration checks.
✔ Package coverage is 81.2%.
✔ R CMD check found no errors.
✔ R CMD check found no warnings.
✔ All applicable standards [v0.0.1] have been documented in this package.
ℹ Current status:
✔ This package may be submitted. |
|
@ropensci-review-bot assign @rkillick as editor |
|
Assigned! @rkillick is now the editor |
|
The goodpractice() checks came back with:
I had problems running covr() but see that it is passing on the checks above. Please fix the above and I'll look to assign reviewers as these are small changes. |
|
Reviewer: @chamberlinc |
|
Reviewer: @brunaw |
|
@ropensci-review-bot add @chamberlinc to reviewers |
|
I'm sorry @rkillick, I'm afraid I can't do that. That's something only editors are allowed to do. |
|
@rkillick I've now added you to the editors team, sorry about this! You can now repeat the comment and it should work. Thanks for your patience! |
|
@ropensci-review-bot add @chamberlinc to reviewers |
|
@chamberlinc added to the reviewers list. Review due date is 2021-11-23. Thanks @chamberlinc for accepting to review! Please refer to our reviewer guide. |
|
@chamberlinc: If you haven't done so, please fill this form for us to update our reviewers records. |
|
@ropensci-review-bot add @brunaw to reviewers |
|
@brunaw added to the reviewers list. Review due date is 2021-11-23. Thanks @brunaw for accepting to review! Please refer to our reviewer guide. |
|
@rkillick @christophsax , thank you for the opportunity to review, and my apologies that this has taken so long. Below is my review with some comments. I also have uploaded the knitted html notebook that details the tests I ran at https://github.com/chamberlinc/tsbox-review. The code I used to test the package functions is also available in this repo. Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
DocumentationThe package includes all the following forms of documentation:
Functionality
Errors encountered in Errors with
Estimated hours spent reviewing: 7
Review CommentsFirst, I enjoyed the opportunity to review this code. This is the first code review I have done for rOpenSci, and I hope the comments below are helpful. I like the general purpose of this package and agree there is probably a lot of use for a package that allows easy conversion between R object types. The documentation I think is all ok, though I think it could be improved with a little more description of the expected structure of data. I did not have any issues with installation. There was one test that failed on my machine (see details in the index.nb.html file in the linked repo, https://github.com/chamberlinc/tsbox-review). The code snippets in the three vignettes all worked on my machine. I will preface my comments by saying I am not an experienced user of data.tables, so following the code was a little tricky for me. I usually work with timeseries as data frames, and I was having trouble following the code and figuring out which columns were being assigned as data, timestamps, or identifiers. This caused a few issues when I had data frames of multiple concurrent timeseries and was getting error messages indicating that the ordering of the columns somehow may have mattered. When working with data frames that only had one timeseries, the functions all worked simply and, it seemed, as intended. I was mostly experimenting with the The code I used to access data and experiment is provided as well at the linked repo. I hope this is helpful! |
|
@chamberlinc thank you for your valuable review of the package. |
|
@brunaw are you able to provide your review soon? It was due on 23 Nov. |
|
@chamberlinc Thank you so much for your review! I will look into it in more detail, but the error messages/feedback in non-standard situations certainly seems worth to be addressed. |
|
First of all, please apologize for my late answer. I originally planned to wait for the second review and lost track afterward. @chamberlinc, thanks again for your very helpful review! I dealt with your points in PR #211. Most of your problems arose when applying the functions to non-standard data frames, especially wide ones. In some cases, the messages already pointed to the use use of A bug in ts_plot() also caused a weird error message. This bug is now resolved as well. To sum up, I addressed the following three points from your review: description of the expected data structure
Extended paragraph on data frames: https://www.tsbox.help/articles/tsbox.html#time-series-in-data-frames-1 Failing test
The error and the warning did not occur on my system, but the computation in Error Messages on wide data frames
I improved some of the error messages when functions were applied to wide data structures. library(nycflights13)
suppressPackageStartupMessages(library(dplyr))
library(tsbox)
packageVersion("tsbox")
#> [1] '0.3.1.9002'
d3 <- weather |>
select(origin, time_hour, temp, humid, precip)
d3
#> # A tibble: 26,115 × 5
#> origin time_hour temp humid precip
#> <chr> <dttm> <dbl> <dbl> <dbl>
#> 1 EWR 2013-01-01 01:00:00 39.0 59.4 0
#> 2 EWR 2013-01-01 02:00:00 39.0 61.6 0
#> 3 EWR 2013-01-01 03:00:00 39.0 64.4 0
#> 4 EWR 2013-01-01 04:00:00 39.9 62.2 0
#> 5 EWR 2013-01-01 05:00:00 39.0 64.4 0
#> 6 EWR 2013-01-01 06:00:00 37.9 67.2 0
#> 7 EWR 2013-01-01 07:00:00 39.0 64.4 0
#> 8 EWR 2013-01-01 08:00:00 39.9 62.2 0
#> 9 EWR 2013-01-01 09:00:00 39.9 62.2 0
#> 10 EWR 2013-01-01 10:00:00 41 59.6 0
#> # … with 26,105 more rowsThis is a wide data frame, so tsbox errors with a meaningful message: ts_ts(d3)
#> Found numeric [id] column(s): 'temp', 'humid'.
#> Are you using a wide data frame? To convert, use 'ts_long()'.
#> Convert column(s) to character or factor to silence this message.
#> [time]: 'time_hour' [value]: 'precip'
#> Error: series has no regular patternThis also works if the order is different d4 <- weather |>
select(origin, temp, humid, precip, time_hour)
d4
#> # A tibble: 26,115 × 5
#> origin temp humid precip time_hour
#> <chr> <dbl> <dbl> <dbl> <dttm>
#> 1 EWR 39.0 59.4 0 2013-01-01 01:00:00
#> 2 EWR 39.0 61.6 0 2013-01-01 02:00:00
#> 3 EWR 39.0 64.4 0 2013-01-01 03:00:00
#> 4 EWR 39.9 62.2 0 2013-01-01 04:00:00
#> 5 EWR 39.0 64.4 0 2013-01-01 05:00:00
#> 6 EWR 37.9 67.2 0 2013-01-01 06:00:00
#> 7 EWR 39.0 64.4 0 2013-01-01 07:00:00
#> 8 EWR 39.9 62.2 0 2013-01-01 08:00:00
#> 9 EWR 39.9 62.2 0 2013-01-01 09:00:00
#> 10 EWR 41 59.6 0 2013-01-01 10:00:00
#> # … with 26,105 more rows
ts_ts(d4)
#> Found numeric [id] column(s): 'temp', 'humid'.
#> Are you using a wide data frame? To convert, use 'ts_long()'.
#> Convert column(s) to character or factor to silence this message.
#> [time]: 'time_hour' [value]: 'precip'
#> Error: series has no regular patternUsing The ts_plot(d3)
#> Found numeric [id] column(s): 'temp', 'humid'.
#> Are you using a wide data frame? To convert, use 'ts_long()'.
#> Convert column(s) to character or factor to silence this message.
#>
#> [time]: 'time_hour' [value]: 'precip'
#> too many series. Only showing the first 20.reflecting the fact that we have a large number of absurdly short time series (since value columns were classified as id columns) I also tried to improve a bit on your examples. I construct a data frame that looks similar to yours: # > str(Conn_discharge_DO)
# 'data.frame': 70519 obs. of 8 variables:
# $ agency_cd : chr "USGS" "USGS" "USGS" "USGS" ...
# $ site_no : chr "01193050" "01193050" "01193050" "01193050" ...
# $ dateTime : POSIXct, format: "2019-01-01 05:00:00" "2019-01-01 05:15:00" "2019-01-01 05:30:00" "2019-01-01 05:45:00" ...
# $ X_00060_00000 : num 39200 41300 40400 40400 41100 43000 41800 42300 41700 42400 ...
# $ X_00060_00000_cd: chr "A" "A" "A" "A" ...
Conn_discharge_DO <- tibble(
agency_cd = c("USGS", "USGS", "USGS", "USGS"),
site_no = c("01193050", "01193050", "01193050", "01193050"),
dateTime = as.POSIXct(c("2019-01-01 05:00:00", "2019-01-01 05:15:00", "2019-01-01 05:30:00", "2019-01-01 05:45:00")),
unit = "ft3/s",
X_00060_00000 = 1:4,
noDataValue = NA,
X_00060_000002 = 11:14,
X_00060_00000_cd = "A"
)tsbox now detects ts_xts(Conn_discharge_DO)
#> Found numeric [id] column(s): 'X_00060_00000'.
#> Are you using a wide data frame? To convert, use 'ts_long()'.
#> Convert column(s) to character or factor to silence this message.
#> [time]: 'dateTime' [value]: 'X_00060_000002'
#> Loading required namespace: xts
#> USGS_01193050_ft3/s_1_NA_A USGS_01193050_ft3/s_2_NA_A
#> 2019-01-01 05:00:00 11 NA
#> 2019-01-01 05:15:00 NA 12
#> 2019-01-01 05:30:00 NA NA
#> 2019-01-01 05:45:00 NA NA
#> USGS_01193050_ft3/s_3_NA_A USGS_01193050_ft3/s_4_NA_A
#> 2019-01-01 05:00:00 NA NA
#> 2019-01-01 05:15:00 NA NA
#> 2019-01-01 05:30:00 13 NA
#> 2019-01-01 05:45:00 NA 14
ts_long(Conn_discharge_DO)
#> found columns right to the [time] column that will be treated as [id] columns (character or factor): 'unit', 'X_00060_00000_cd'.
#> Additional [id] column(s): 'agency_cd', 'site_no', 'unit', 'X_00060_00000_cd'
#> [time]: 'dateTime'
#> # A tibble: 12 × 7
#> agency_cd site_no unit X_00060_00000_cd id dateTime value
#> <chr> <chr> <chr> <chr> <chr> <dttm> <int>
#> 1 USGS 01193050 ft3/s A X_00060_… 2019-01-01 05:00:00 1
#> 2 USGS 01193050 ft3/s A X_00060_… 2019-01-01 05:15:00 2
#> 3 USGS 01193050 ft3/s A X_00060_… 2019-01-01 05:30:00 3
#> 4 USGS 01193050 ft3/s A X_00060_… 2019-01-01 05:45:00 4
#> 5 USGS 01193050 ft3/s A noDataVa… 2019-01-01 05:00:00 NA
#> 6 USGS 01193050 ft3/s A noDataVa… 2019-01-01 05:15:00 NA
#> 7 USGS 01193050 ft3/s A noDataVa… 2019-01-01 05:30:00 NA
#> 8 USGS 01193050 ft3/s A noDataVa… 2019-01-01 05:45:00 NA
#> 9 USGS 01193050 ft3/s A X_00060_… 2019-01-01 05:00:00 11
#> 10 USGS 01193050 ft3/s A X_00060_… 2019-01-01 05:15:00 12
#> 11 USGS 01193050 ft3/s A X_00060_… 2019-01-01 05:30:00 13
#> 12 USGS 01193050 ft3/s A X_00060_… 2019-01-01 05:45:00 14 |
|
Hello Christoph, The details of the automated test output you can find here: https://github.com/chamberlinc/tsbox-review/blob/master/tsbox_R2.md. The rest of my review is below: Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
DocumentationThe package includes all the following forms of documentation:
Functionality
Estimated hours spent reviewing: 3
Review CommentsThe functions now work as expected for me for the types of data I typically use. Thanks for the updates! I found the messages on the ts_long function to be very helpful. |
|
Very sorry for the slow turnaround. Native PipeI changed the native pipe ( I introduced the native pipe deliberately in the vignettes since I always wanted to keep the dependency graph very simple (tsbox only imports data.table and anytime) and wanted to make the vignettes available for non-magrittr/dplyr users. But I see that there is a trade-off between this aim and the support for older versions of R. The native pipe was introduced in R 4.1 on 2021-05-18 and is now available for almost 15 months. With more time passing, only a few older versions will be around. I changed it back for now but will go to the native pipe again at some point in the future. Other Test ErrorsI have a hard time reproducing them. First, all the tests run through on my system (macOS, R release) and all systems on GitHub Actions (ubuntu-20.04, release, devel), (windows-latest, release), (macOS-latest, release). So I wonder if this is another problem related to the old R version? Your error says: Error in `UseMethod("ts_dts")`: no applicable method for 'ts_dts' applied to an object of class "data.frame"But there clearly is such a method: If you still encounter the error in the tests, could you try to run one the failing example outside of the tests? library(tsbox)
with_id <- wo_id <- ts_df(mdeaths)
with_id$id <- "mdeaths"
ts_c(wo_id, with_id)Thank you so much for your work! I guess we are alomost done now. I will answer quickly this time :-) |
Submitting Author Name: Christoph Sax
Due date for @chamberlinc: 2021-11-23Submitting Author Github Handle: @christophsax
Repository: https://github.com/christophsax/tsbox
Version submitted: 3.1.9001
Submission type: Stats
Badge grade: silver
Editor: @rkillick
Reviewers: @chamberlinc, @brunaw
Due date for @brunaw: 2021-11-23
Archive: TBD
Version accepted: TBD
Pre-submission Inquiry
General Information
Anyone who works with time series. Many statistical packages require time series to be in a certain object (ts, xts, tsibble, data.frame). tsbox facilitates the conversion between these objects. It also provides a general toolkit that works the same way with all time series classes. {tsbox} is also mentioned in the rOpenSci Statistical Software Peer Review Section on Time Series.
Paste your responses to our General Standard G1.1 here, describing whether your software is:
In the rOpenSci classification, this package is An improvement on other implementations of similar algorithms in R. Many time series packages, e.g., zoo or tsibble contain converter functions from one class to another. They often convert from their class to
tsobjects and back, but lack converters to other time series class.In most cases, tsbox transforms an object into an augmented
data.table. And uses thedata.tableinfrastructure for efficient joining and reshaping. After computation, it restores the original input class. This restoring feature iswas also used in the
xts::reclass()function of the xts package.data.table: For efficient joining and reshaping
xts: Similar reclassing mechanism
tsibble: Tidy Temporal Data Frames and Tools
Not applicable.
Badging
I probably need some advice on this. I think that tsbox complies with most standards that are applicable. Generality of usage is a particular feature that should qualify the package for silver.
The most outstanding point is probably the one on generality:
The package facilitates work with time series in general. It can also be used to ease the burden of object testing and time series conversion for other time series packages.
Technical checks
Confirm each of the following by checking the box.
autotestchecks on the package, and ensured no tests fail.srr_stats_pre_submit()function confirms this package may be submitted.This package:
Publication options
Do you intend for this package to go on CRAN?
Already published on CRAN.
Do you intend for this package to go on Bioconductor?
Code of conduct
The text was updated successfully, but these errors were encountered: