Remove `TODO` comment in `_pydatetime._isoweek1monday`

[bombs-kim]

Feature or enhancement

Proposal:

According to the existing note # XXX This could be done more efficiently, the _isoweek1monday function can be improved.

Current Implementation:

def _isoweek1monday(year):
    # Helper to calculate the day number of the Monday starting week 1
    # XXX This could be done more efficiently
    THURSDAY = 3
    firstday = _ymd2ord(year, 1, 1)
    firstweekday = (firstday + 6) % 7  # See weekday() above
    week1monday = firstday - firstweekday
    if firstweekday > THURSDAY:
        week1monday += 7
    return week1monday

Proposed Change

Replace the current implementation with a simplified logic that is both more performant and easier to understand.

def _isoweek1monday_new(year):
    # Calculate the ordinal day of the Monday starting ISO week 1.
    # ISO week 1 is defined as the week that contains January 4th.
    jan4 = _ymd2ord(year, 1, 4)
    jan4_weekday = (jan4 + 6) % 7
    return jan4 - jan4_weekday

Rationale

The new logic:
1. Aligns directly with the ISO week definition, which makes it easier to comprehend.
2. Reduces unnecessary computations, improving performance slightly.

Validation and Performance Comparison

Using timeit to compare the current and proposed implementations:

import timeit
from _pydatetime import _isoweek1monday, _ymd2ord

def _isoweek1monday_new(year):
    jan4 = _ymd2ord(year, 1, 4)
    jan4_weekday = (jan4 + 6) % 7
    return jan4 - jan4_weekday

# Validation results:
# >>> all(_isoweek1monday_new(i) == _isoweek1monday(i) for i in range(2000))
# True

# Timing results:
# >>> timeit.timeit('_isoweek1monday(2000)', globals=globals())
# 0.4805744589539245
# >>> timeit.timeit('_isoweek1monday_new(2000)', globals=globals())
# 0.4299807079951279

Has this already been discussed elsewhere?

No response given

Links to previous discussion of this feature:

No response

Linked PRs

• gh-127553: Remove TODO comment in _pydatetime._isoweek1monday #127564
• [3.13] gh-127553: Remove outdated TODO comment in _pydatetime (GH-127564) #128500
• [3.12] gh-127553: Remove outdated TODO comment in _pydatetime (GH-127564) #128501

[picnixz]

Using timeit to compare the current and proposed implementations:

Out of curiosity, is it a debug build or a release+PGO+LTO build? Usually, pyperf to check performances is a way to check for the variance of the results. Also, instead of timeit.timeit, can you use https://docs.python.org/3/library/timeit.html#command-line-interface in order to have the running time per loop? thanks

[bombs-kim]

@picnixz I've used the latest debug build.

Here I share the result of running cli timeit

↪ ./python.exe -m timeit -s "from timeit_test import _isoweek1monday" "_isoweek1monday(2000)"
500000 loops, best of 5: 407 nsec per loop

↪ ./python.exe -m timeit -s "from timeit_test import _isoweek1monday_new" "_isoweek1monday_new(2000)"
500000 loops, best of 5: 403 nsec per loop

[picnixz]

Ok, that's what I suspect. I'm not sure it's worth the change for the performance part, but it could be worth the change to remove the XXX comment.

[bombs-kim]

@picnixz Okay, thanks for the performance review! Apart from the performance, I thought that the new logic is more succinct and easier to understand. Can you agree that the new logic read better?

[picnixz]

Correctness seems to work (I tried for -50000 to +50000) but I haven't done the maths (if someone has time for that, I'd be happy to). Because the condition > 3 which adds +7 seems to be handled properly.

[erlend-aasland]

Apart from the performance, I thought that the new logic is more succinct and easier to understand. Can you agree that the new logic read better?

We seldom do these kinds of refactorings. If we did, every now and then someone who disagrees with the former refactoring would appear, and you'd get a constant urge for new refactorings and rewrites. If the code works, let it be. Ditto for code style changes.

[bombs-kim]

Thanks for the feedback, @erlend-aasland. I get your point about avoiding unnecessary refactorings or changes that might lead to constant revisits.

In this case, my primary goal was to address the # XXX comment, as it implies room for improvement. Since the proposed change simplifies the logic without altering functionality or style significantly, I thought it could be worth removing the XXX note and replacing the implementation with something clearer.

However, if the preference is to leave the code as is, I’m happy to just remove the XXX comment and keep the current implementation unchanged. Please let me know if that works for you!

[picnixz]

I thought that the new logic is more succinct and easier to understand

Sorry I didn't reply to this one. The current code has the benefit of being clear in what it does without relying on the fact that some week contains some specific day. But your comment makes it a bit clearer. So I'd say the two implementations weigh the same and I'd prefer just leaving something that works as is.

The only thing I would be willing to accept is removing the comment so that future people don't think there is still work to do.