Skip to content

Use faster base implementation for isoweek#7144

Merged
MichaelChirico merged 2 commits intomasterfrom
isoweek-fast
Jul 10, 2025
Merged

Use faster base implementation for isoweek#7144
MichaelChirico merged 2 commits intomasterfrom
isoweek-fast

Conversation

@MichaelChirico
Copy link
Copy Markdown
Member

Closes #5111.

Matt's comment about trying to make isoweek() even faster than the base implementation is nice, but in the meantime, we shouldn't miss out on the 20x speed-up.

I'm fairly confident #3279 will help a lot in this case -- the slowest step in the master implementation is converting all the Jan. 1 in the input from character with as.IDate(). Presumably there will only be a few unique Jan. 1 in the input, meaning we only really need to convert a few unique dates with the slow strptime() path.

But anyway, that can be done later for a second, further speed-up.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.50%. Comparing base (038e7f8) to head (1afb359).
Report is 6 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7144      +/-   ##
==========================================
- Coverage   98.50%   98.50%   -0.01%     
==========================================
  Files          81       81              
  Lines       15015    15014       -1     
==========================================
- Hits        14791    14790       -1     
  Misses        224      224              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jul 8, 2025

No obvious timing issues in HEAD=isoweek-fast
Comparison Plot

Generated via commit 1afb359

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

Task Duration
R setup and installing dependencies 2 minutes and 46 seconds
Installing different package versions 39 seconds
Running and plotting the test cases 2 minutes and 52 seconds

@ben-schwen
Copy link
Copy Markdown
Member

ben-schwen commented Jul 10, 2025

LGTM, I only question if we should also add isoyear = function(x) as.integer(format(as.IDate(x), "%G"))?

Since isoyear("2019-12-30") is 2020.

@MichaelChirico MichaelChirico mentioned this pull request Jul 10, 2025
@MichaelChirico
Copy link
Copy Markdown
Member Author

Good point -- it also came up in #2611. But as we don't have it yet, I'll mark it for follow-up: #7154.

@MichaelChirico MichaelChirico merged commit ed2df98 into master Jul 10, 2025
12 checks passed
@MichaelChirico MichaelChirico deleted the isoweek-fast branch July 10, 2025 16:56
@MichaelChirico MichaelChirico restored the isoweek-fast branch July 10, 2025 17:33
@MichaelChirico MichaelChirico added the atime Requests related to adding/improving/monitoring performance regression tests via atime. label Jul 10, 2025
@jangorecki jangorecki deleted the isoweek-fast branch September 27, 2025 09:17
@MichaelChirico MichaelChirico restored the isoweek-fast branch October 20, 2025 16:53
@tdhock tdhock deleted the isoweek-fast branch April 21, 2026 03:29
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.

Just use base functionality for isoweek?

2 participants