Remove `TODO` comment in `_pydatetime._isoweek1monday` · Issue #127553 · python/cpython · GitHub

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

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

@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

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.

@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?

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.

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.

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!

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.

Code and comment are by Alexander Belopolsky, no longer active.

To me, timings with normal release non-debug builds are more relevant for production use. Is the 'cli timit' such?

Thank you for the feedback and insights, everyone! I realize I made an oversight in my earlier performance testing. I apologize for any confusion caused and truly appreciate the time and effort you’ve spent reviewing this. Based on @terryjreedy’s suggestion, I have re-evaluated the performance with additional tests on both debug and optimized builds. Below are the results:

Performance Tests

Profiling for Single Input (Year = 2000)

Without Optimizations

_isoweek1monday_old: 447 nsec per loop
_isoweek1monday_new: 411 nsec per loop

With Optimizations (./configure --enable-optimizations)

_isoweek1monday_old: 544 nsec per loop
_isoweek1monday_new: 500 nsec per loop

Profiling for Various Inputs (Years 1900-2100)

Without Optimizations

run_old(): 89 µs per loop
run_new(): 82.8 µs per loop

With Optimizations

run_old(): 108 µs per loop
run_new(): 101 µs per loop

Using pyperf for More Robust Results

Without Optimizations

run old: Mean +- std dev: 90.5 us +- 2.6 us
run new: Mean +- std dev: 83.2 us +- 1.5 us

With Optimizations (./configure --enable-optimizations)

run_old(): 109 µs ± 3 µs
run_new(): 102 µs ± 1 µs

Observations

Performance: The new implementation demonstrates improvements in both debug and optimized builds. Interestingly, the release build showed worse performance for my test code; nevertheless, the change consistently outperforms the existing implementation.
Clarity: I appreciate @picnixz’s earlier point that the current implementation benefits from being explicit in its operations and avoids relying on the fact that ISO week 1 always contains January 4th. This is a valid perspective that prioritizes transparency in logic; however, it still requires knowledge of the special case where Thursday serves as the anchor point.
I still believe the proposed implementation offers its own form of clarity. Directly referencing January 4th as the anchor for ISO week 1, it closely aligns with the ISO definition and eliminates the need for an additional condition (if firstweekday > THURSDAY). At the very least, the proposed change requires shorter code.

Closing Remarks

Given the feedback and results:

The new implementation improves clarity in one sense but could be seen as relying more on the ISO week definition, which not everyone might find preferable.
If the preference is for minimal disruption, I can simply remove the # XXX comment to avoid implying that work remains to be done on this function.

I remain open to community consensus on whether the refactoring is worth it or if we should only focus on removing the TODO comment. Let me know your thoughts!

Test Code Used and Environment

MacBook Pro 14-inch, 2021
Chip Apple M1 Pro
Memory 16 GB
OS macOS 15.1.1 Sequoia (24B91)

# timeit_test.py
from _pydatetime import _isoweek1monday as _isoweek1monday_old, _ymd2ord


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


def run_old():
    for year in range(1900, 2100):
        assert _isoweek1monday_old(year)


def run_new():
    for year in range(1900, 2100):
        assert _isoweek1monday_new(year)


# import pyperf

# runner = pyperf.Runner()
# runner.timeit(name="run old", stmt="run_old()", globals=globals())
# runner.timeit(name="run new", stmt="run_new()", globals=globals())

./python.exe -m timeit -s "from timeit_test import _isoweek1monday_old" "_isoweek1monday_old(2000)"
./python.exe -m timeit -s "from timeit_test import _isoweek1monday_new" "_isoweek1monday_new(2000)"
./python.exe -m timeit -s "from timeit_test import run_new" "run_new()"
./python.exe -m timeit -s "from timeit_test import run_old" "run_old()"

I'm a bit conflicted. On one's side, we do have a slight improvement but on the other, this only concerns the python implementation. I'm not aware whether non-CPython implementations uses that implementation (maybe PyPy?) but I wouldn't try optimizing Python code that only exists as a fallback.

It would be much more meaningful if you can improve the C implementation of this (namely how do we do it in C? is there an equivalent improvement that could be brought?)

gh-127553: Remove outdated TODO comment in _pydatetime (#127564)

pythongh-127553: Remove outdated TODO comment in _pydatetime (pythonG…

[3.12] gh-127553: Remove outdated TODO comment in _pydatetime (GH-127564

[3.13] gh-127553: Remove outdated TODO comment in _pydatetime (GH-127564

pythongh-127553: Remove outdated TODO comment in _pydatetime (python#…

Remove `TODO` comment in `_pydatetime._isoweek1monday` · Issue #127553 · python/cpython · GitHub

Repository navigation

Remove TODO comment in _pydatetime._isoweek1monday #127553

Feature or enhancement

Proposal:

Current Implementation:

Proposed Change

Rationale

Validation and Performance Comparison

Has this already been discussed elsewhere?

Links to previous discussion of this feature:

Linked PRs

5 remaining items

Performance Tests

Profiling for Single Input (Year = 2000)

Without Optimizations

With Optimizations (./configure --enable-optimizations)

Profiling for Various Inputs (Years 1900-2100)

Without Optimizations

With Optimizations

Using pyperf for More Robust Results

Without Optimizations

With Optimizations (./configure --enable-optimizations)

Observations

Closing Remarks

Test Code Used and Environment

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Remove `TODO` comment in `_pydatetime._isoweek1monday` · Issue #127553 · python/cpython · GitHub

Repository navigation

Remove TODO comment in _pydatetime._isoweek1monday #127553

Description

Feature or enhancement

Proposal:

Current Implementation:

Proposed Change

Rationale

Validation and Performance Comparison

Has this already been discussed elsewhere?

Links to previous discussion of this feature:

Linked PRs

Activity

picnixz commented on Dec 3, 2024

bombs-kim commented on Dec 3, 2024

picnixz commented on Dec 3, 2024

bombs-kim commented on Dec 3, 2024

picnixz commented on Dec 3, 2024

erlend-aasland commented on Dec 3, 2024

bombs-kim commented on Dec 3, 2024

picnixz commented on Dec 3, 2024

5 remaining items

terryjreedy commented on Dec 3, 2024

bombs-kim commented on Dec 4, 2024

Performance Tests

Profiling for Single Input (Year = 2000)

Without Optimizations

With Optimizations (./configure --enable-optimizations)

Profiling for Various Inputs (Years 1900-2100)

Without Optimizations

With Optimizations

Using pyperf for More Robust Results

Without Optimizations

With Optimizations (./configure --enable-optimizations)

Observations

Closing Remarks

Test Code Used and Environment

picnixz commented on Dec 5, 2024

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions