Skip to content

Use merge commits for atime tests + cleanup#7701

Open
tdhock wants to merge 13 commits intomasterfrom
atime-merge
Open

Use merge commits for atime tests + cleanup#7701
tdhock wants to merge 13 commits intomasterfrom
atime-merge

Conversation

@tdhock
Copy link
Copy Markdown
Member

@tdhock tdhock commented Apr 8, 2026

Closes #7363

Change an atime historical commit from a commit inside a PR branch, to the merge commit of that PR, and deleted branches, in

also some more general atime cleanup which should not affect test results:

  • reduce density of elements in N (reduce false positives)
  • move expr to last arg of atime_test, so easy to comment historical versions.
  • added and fixed comments to document commit ID sources.

Toby Dylan Hocking added 2 commits April 8, 2026 11:24
@tdhock tdhock requested a review from Anirban166 as a code owner April 8, 2026 15:28
@tdhock tdhock changed the title Atime merge Use merge commits for atime tests Apr 8, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 8, 2026

  • HEAD=atime-merge much slower for isoweek improved in #7144
  • HEAD=atime-merge slower P<0.001 for setDT improved in #5427
    Comparison Plot

Generated via commit c9ebb95

Download link for the artifact containing the test results: ↓ atime-results.zip

Task Duration
R setup and installing dependencies 3 minutes and 2 seconds
Installing different package versions 4 minutes and 8 seconds
Running and plotting the test cases 6 minutes and 52 seconds

@tdhock tdhock marked this pull request as draft April 8, 2026 16:52
@tdhock tdhock added the atime Requests related to adding/improving/monitoring performance regression tests via atime. label Apr 13, 2026
@tdhock tdhock marked this pull request as ready for review April 21, 2026 03:55
@tdhock tdhock changed the title Use merge commits for atime tests Use merge commits for atime tests + cleanup Apr 21, 2026
@tdhock
Copy link
Copy Markdown
Member Author

tdhock commented Apr 21, 2026

I checked atime results before and after this PR, and they look consistent. The biggest difference is in this test, which is normal because old test code was starting at a custom N=1e3 whereas new test code uses the standard N=1e1 for this case (custom N code deleted).
image

Above is a screenshot from
image

which I made using data from

library(data.table)
bench_dt <- data.table(test_code=c("old","new"))[, {
  result_dir <- paste0("atime-results-",test_code)
  tests.RData <- file.path(result_dir, "tests.RData")
  load(tests.RData)
  bench.dt[, .(Test, unit, N, empirical, expr.name)]
}, by=test_code]

library(ggplot2)
gg <- ggplot()+
  geom_line(aes(
    N, empirical, color=expr.name),
    data=bench_dt)+
  scale_color_manual(values=atime:::default.version.colors)+
  facet_grid(unit ~ Test + test_code, scales="free", labeller=label_both)+
  scale_x_log10()+
  scale_y_log10()
png("atime-results.png", width=120, height=8, units="in", res=100)
print(gg)
dev.off()

@tdhock
Copy link
Copy Markdown
Member Author

tdhock commented Apr 21, 2026

I updated https://github.com/Rdatatable/data.table/wiki/Performance-testing to mention

  • only use merge commits on master (not other commits in PR branches which will be deleted)
  • put expr as last argument of atime_test() so we can easily comment other args.

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

Labels

atime Requests related to adding/improving/monitoring performance regression tests via atime.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

allow to fail atime job in GHCI / use merge commits to allow branch deletion

1 participant