Skip to content

Replace datetime::timedelta with custom function#1785

Merged
arkid15r merged 3 commits into
vacanza:devfrom
KJhellico:ref-replace-td
May 9, 2024
Merged

Replace datetime::timedelta with custom function#1785
arkid15r merged 3 commits into
vacanza:devfrom
KJhellico:ref-replace-td

Conversation

@KJhellico

Copy link
Copy Markdown
Collaborator

Proposed change

Replace datetime::timedelta with simple custom function for adding and subtracting days.

Performance test script:

from datetime import date, timedelta
import timeit

def _delta_days(dt: date, n: int) -> date:
    return date.fromordinal(dt.toordinal() + n)

def with_timedelta():
    dt = date(2024, 1, 1)
    d2 = dt + timedelta(days=10)

def with_delta_days():
    dt = date(2024, 1, 1)
    d2 = _delta_days(dt, 10)

NUM = 10_000_000

t1 = timeit.timeit(with_timedelta, number=NUM)
t2 = timeit.timeit(with_add_days, number=NUM)

print(f"{NUM} times:")
print(f"timedelta = {t1:.3f}")
print(f"_delta_days = {t2:.3f} ({(t2/t1 - 1)*100:.2f}%)")

Result:

timedelta = 5.492
_add_days = 3.810 (-30.63%)

Type of change

  • New country/market holidays support (thank you!)
  • Supported country/market holidays update (calendar discrepancy fix, localization)
  • Existing code/documentation/test/process quality improvement (best practice, cleanup, refactoring, optimization)
  • Dependency update (version deprecation/upgrade)
  • Bugfix (non-breaking change which fixes an issue)
  • Breaking change (a code change causing existing functionality to break)
  • New feature (new python-holidays functionality in general)

Checklist

  • I've followed the contributing guidelines
  • I've run make pre-commit, it didn't generate any changes
  • I've run make test, all tests passed locally

@coveralls

coveralls commented May 4, 2024

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 9018175993

Details

  • 104 of 104 (100.0%) changed or added relevant lines in 25 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 8972750703: 0.0%
Covered Lines: 11064
Relevant Lines: 11064

💛 - Coveralls

@arkid15r arkid15r left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This would be another win for us on the performance grounds.

After glancing this I wonder if we could use some sort of a lightweight timedelta class version keeping all the syntax (add/radd methods with days support only).
Of course this would make sense if the performance gain doesn't degrade.

@KJhellico do you think it's worth a shot?

@KJhellico

Copy link
Copy Markdown
Collaborator Author

I tried this, but there is no performance gain.

from datetime import date, timedelta
import timeit

class td:
    def __init__(self, days: int = 0):
        self._days = days

    def __radd__(self, other):
        if isinstance(other, date):
            return date.fromordinal(other.toordinal() + self._days)
        return NotImplemented

    def __rsub__(self, other):
        if isinstance(other, date):
            return date.fromordinal(other.toordinal() - self._days)
        return NotImplemented


def _start_days(dt: date, n: int) -> date:
    return date.fromordinal(dt.toordinal() + n)

def with_timedelta():
    dt = date(2024, 1, 1)
    d2 = dt + timedelta(days=10)

def with_start_days():
    dt = date(2024, 1, 1)
    d2 = _start_days(dt, 10)

def with_class_td():
    dt = date(2024, 1, 1)
    d2 = dt + td(10)

NUM = 10_000_000

t1 = timeit.timeit(with_timedelta, number=NUM)
t2 = timeit.timeit(with_start_days, number=NUM)
t3 = timeit.timeit(with_class_td, number=NUM)

print(f"{NUM} times:")
print(f"timedelta = {t1:.3f}")
print(f"_start_days = {t2:.3f} ({(t2/t1 - 1)*100:.2f}%)")
print(f"class td = {t3:.3f} ({(t3/t1 - 1)*100:.2f}%)")

Result:

10000000 times:
timedelta = 5.534
_start_days = 3.749 (-32.25%)
class td = 5.505 (-0.52%)

@arkid15r arkid15r left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I tried this, but there is no performance gain.

Thanks for sharing that! I'd definitely prefer performance over readability here. So our best next step would be to use the function based approach. I have just one naming/flexibility suggestion on this.

The rest LGTM, great PR!

Comment thread holidays/calendars/gregorian.py Outdated
@sonarqubecloud

sonarqubecloud Bot commented May 9, 2024

Copy link
Copy Markdown

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@arkid15r arkid15r left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@arkid15r arkid15r enabled auto-merge May 9, 2024 16:03
@arkid15r arkid15r added this pull request to the merge queue May 9, 2024
Merged via the queue into vacanza:dev with commit 0d4dcec May 9, 2024
@KJhellico KJhellico deleted the ref-replace-td branch May 9, 2024 16:10
@arkid15r arkid15r mentioned this pull request May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants