Skip to content
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

adding an atime test case; new with optimization to allow avoid [ overhead #PR4488 #6290

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

DorisAmoakohene
Copy link
Contributor

This issue was reported in #3735. The issue was reported that, selecting by row number from a data.table is much slower than from a data.frame.

The pull request that fixed the regression can be found #4488. However, I couldn’t find specific details about the changes made to fix the regression. It’s likely that the changes involved optimizing the way data.table handles row selection to make it more efficient.

Copy link

github-actions bot commented Jul 17, 2024

Comparison Plot

Generated via commit e23e865

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

Time taken to finish the standard R installation steps: 3 minutes and 18 seconds

Time taken to run atime::atime_pkg on the tests: 7 minutes and 28 seconds

.ci/atime/tests.R Outdated Show resolved Hide resolved
@tdhock
Copy link
Member

tdhock commented Jul 17, 2024

please revise #6288, and update this PR using the same advice in that other PR, before submitting more PRs

.ci/atime/tests.R Outdated Show resolved Hide resolved
@tdhock tdhock marked this pull request as draft July 18, 2024 02:35
@tdhock
Copy link
Member

tdhock commented Jul 18, 2024

converting to draft so we can focus on #6288 first

@DorisAmoakohene DorisAmoakohene marked this pull request as ready for review July 29, 2024 18:36
@DorisAmoakohene
Copy link
Contributor Author

converting to draft so we can focus on #6288 first

@tdhock could you please review this PR for merge also

@MichaelChirico MichaelChirico added the atime Requests related to adding/improving performance regression tests via atime label Sep 8, 2024
Copy link
Member

@Anirban166 Anirban166 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requires revision (and the plot doesn't seem to be showing improvement)

DoSomething <- function(row) {
someCalculation <- row[["v1"]] + 1
}
allIteration_dt <- as.data.table(allIterations)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the point of this line? it's not being used anywhere inside the evaluated expression, noh? (expr is only using allIterations)

setup = {
allIterations <- data.frame(v1 = runif(N), v2 = runif(N))
DoSomething <- function(row) {
someCalculation <- row[["v1"]] + 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs indentation within this scope

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 performance regression tests via atime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants