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

Fixed inconsistencies in .ci/atime/tests.R #6492

Merged
merged 2 commits into from
Sep 10, 2024
Merged

Conversation

Anirban166
Copy link
Member

I noticed some minor inconsistencies in the file, for e.g. double expr from #6288: (also, using curly braces for only multiple lines might be good for consistency)

expr = {
expr=data.table:::`[.data.table`(d, , max(v1) - min(v2), by = id3)
},

Originally posted by @Anirban166 in #6464 (comment)

Also, some comments needed corrections, such as:

Fast = "1872f473b20fdcddc5c1b35d79fe9229cd9a1d15"), # Last commit in the PR that fixes the issue (https://github.com/Rdatatable/data.table/pull/5427/commits)

(third-last commit, not last)

@Anirban166 Anirban166 requested a review from tdhock as a code owner September 10, 2024 02:35
Copy link

github-actions bot commented Sep 10, 2024

Comparison Plot

Generated via commit ef88d6f

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

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

Time taken to run atime::atime_pkg on the tests: 6 minutes and 35 seconds

@Anirban166
Copy link
Member Author

@MichaelChirico @tdhock Seems like the setup time decrease is worthwhile/significant! :D
(This is showing results that are consistent with what I'm getting on my fork now, for e.g. the first run there was v1.2.0 taking ~ 12 minutes for setup vs v1.2.1 now taking just 3 and a half minutes)

Slow = "c4a2085e35689a108d67dacb2f8261e4964d7e12", # Parent of the first commit in the PR that fixes the issue (https://github.com/Rdatatable/data.table/commit/7cc4da4c1c8e568f655ab5167922dcdb75953801)
Fast = "1872f473b20fdcddc5c1b35d79fe9229cd9a1d15"), # Last commit in the PR that fixes the issue (https://github.com/Rdatatable/data.table/pull/5427/commits)
Slow = "c4a2085e35689a108d67dacb2f8261e4964d7e12", # Parent of the first commit (https://github.com/Rdatatable/data.table/commit/7cc4da4c1c8e568f655ab5167922dcdb75953801) in the PR (https://github.com/Rdatatable/data.table/pull/5427/commits) that fixes the issue
Fast = "1872f473b20fdcddc5c1b35d79fe9229cd9a1d15"), # Antepenultimate commit in the PR (https://github.com/Rdatatable/data.table/pull/5427/commits) that fixes the issue
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand Antepenultimate, why did you change from the simpler "last" ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The SHA is not associated with the last commit (you can check the link and verify), but I agree that it's better to avoid such a jargon - will 'third-last' or 'close-to-last' work?

Copy link
Member

Choose a reason for hiding this comment

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

Is there something wrong with the last SHA in the PR, then just "last commit" will be correct? otherwise, third-to-last seems best to me (as much as I hate wasting such a perfectly valid word!)

Copy link
Member Author

Choose a reason for hiding this comment

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

Great point! No issues with the SHA, just didn't get updated at the end of #6094 unfortunately (I should've tried it with the last one before changing the comment)

Thank you both!

@MichaelChirico MichaelChirico added ci atime Requests related to adding/improving performance regression tests via atime labels Sep 10, 2024
@tdhock tdhock merged commit 8cc2dd1 into master Sep 10, 2024
8 checks passed
@Anirban166 Anirban166 deleted the consistent-atime-tests branch September 23, 2024 21:58
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 ci
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants