Moved tests to the more appropriate directory and added a test case based on the performance improvement to be brought by #5427 #6094
Conversation
…e, added a test, renamed test case titles
|
thanks! |
|
Also I see Fast/Slow in grey, which should be fixed by my recent PR to atime https://github.com/tdhock/atime/pull/48/files (not yet on CRAN, plan to submit next week, after which time we can consider merging this PR) |
|
Generated via commit 6c7157d Download link for the artifact containing the test results: ↓ atime-results.zip Time taken to finish the standard R installation steps: 11 minutes and 40 seconds Time taken to run |
Yup I made the changes and we'll get to see the plot soon (two times as I made two pushes; although for one workflow or
Perfect! |
|
Just saw the plot, not sure why |
@Anirban166 how will faceting work if there are, e.g. 10 plots? will it wind up being 2x20 plot or will it start breaking into new rows soon? |
Good question in terms of visual scalability. I agree (if you're thinking the same) that we would likely want to vertically append those subplots too if it's too much on the horizontal. I'll need to investigate, but the answer should be in the code that generates these plots in |
|
@MichaelChirico I looked it up and it seems that adding to the test cases will keep on appending horizontally only as per this line: (and for each test case the time and memory plots are vertically arranged as per this line) ggplot2::facet_grid(unit ~ expr.name, scales="free")So yes, at present it'll be 2x20 for twenty test cases with each row only corresponding to a different unit for each of those columns. |
Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>
|
I filed tdhock/atime#49 to handle this first upstream, nothing to do in data.table for now. |
merge-base is missing if it is the same as another commit already shown (typically master) |
|
why does the Fast curve memory go up so sharply for large N? -> answering my own question: |
| @@ -105,6 +105,18 @@ test.list <- list( | |||
| expr = quote(data.table:::`[.data.table`(dt_mod, , N := .N, by = g)), | |||
| Before = "be2f72e6f5c90622fe72e1c315ca05769a9dc854", # Parent of the regression causing commit (https://github.com/Rdatatable/data.table/commit/e793f53466d99f86e70fc2611b708ae8c601a451) in the PR that introduced the issue (https://github.com/Rdatatable/data.table/pull/4491/commits) | |||
| Regression = "e793f53466d99f86e70fc2611b708ae8c601a451", # Commit responsible for regression in the PR that introduced the issue (https://github.com/Rdatatable/data.table/pull/4491/commits) | |||
| Fixed = "58409197426ced4714af842650b0cc3b9e2cb842") # Last commit in the PR that fixed the regression (https://github.com/Rdatatable/data.table/pull/5463/commits) | |||
| Fixed = "58409197426ced4714af842650b0cc3b9e2cb842"), # Last commit in the PR that fixed the regression (https://github.com/Rdatatable/data.table/pull/5463/commits) | |||
There was a problem hiding this comment.
Another aside: @tdhock let's support test.list having a NULL final entry, that way we can have like:
test.list <- list(
"A" = list(...),
"B" = list(...),
NULL
)And avoid the pesky git diff on irrelevant lines every time a test is added (as in here, where we add , --> diff)
There was a problem hiding this comment.
implemented in atime github, https://github.com/tdhock/atime/blob/ea591e37a8fbf69a06a0cdbb8a51108cb73290fb/R/test.R#L14 I will file a follow up PR next week after that gets on CRAN
A follow-up to #6078 that:
.ciand correspondingly updates the version of my action in theperformance-testsworkflow to accommodate that change.(For reference, the current state of the plot with these test cases looks like this)