[issue24416] Have date.isocalendar() return a structseq instance

Baptiste Mispelon added the comment: (Apologies if this isn't the right place and/or time for this kind of negative feedback. I'm open to suggestions for a more appropriate venue) I found it disheartening that my work on this ticket has been erased. While I understand that other

[issue24416] Have date.isocalendar() return a structseq instance

Petr Viktorin added the comment: This broke compilation with mingw; see https://bugs.python.org/issue40777 -- nosy: +petr.viktorin ___ Python tracker ___

[issue24416] Have date.isocalendar() return a structseq instance

Paul Ganssle added the comment: This is now merged, thanks for the debate and opinions offered everyone, and thanks to Dong-hee for the implementation! The way we did the implementation, a pickle/unpickle cycle on the result of .isocalendar() will return a plain tuple. Despite the fact that

[issue24416] Have date.isocalendar() return a structseq instance

Paul Ganssle added the comment: New changeset 1b97b9b0ad9a2ff8eb5c8f2e2e7c2aec1d13a330 by Paul Ganssle in branch 'master': bpo-24416: Return named tuple from date.isocalendar() (GH-20113) https://github.com/python/cpython/commit/1b97b9b0ad9a2ff8eb5c8f2e2e7c2aec1d13a330 --

[issue24416] Have date.isocalendar() return a structseq instance

Change by Paul Ganssle : -- pull_requests: +19420 pull_request: https://github.com/python/cpython/pull/20113 ___ Python tracker ___

[issue24416] Have date.isocalendar() return a structseq instance

Change by STINNER Victor : -- nosy: -vstinner ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe:

[issue24416] Have date.isocalendar() return a structseq instance

Change by Dong-hee Na : -- pull_requests: +17118 pull_request: https://github.com/python/cpython/pull/17651 ___ Python tracker ___

[issue24416] Have date.isocalendar() return a structseq instance

Paul Ganssle added the comment: The current state of the PR doesn't hinge on the pure Python implementation, we went with a very simple tuple subclass to keep the two more closely in sync and because we don't need any of the additional functionality that namedtuple brings, but if it were

[issue24416] Have date.isocalendar() return a structseq instance

Tim Peters added the comment: I agree with Raymond here: using collections.namedtuple is fine in the pure Python version. Since Raymond checked in doc changes to purge the phrase "struct sequences" (thanks again for that!), it's consistent with everything else now for the new datetime

[issue24416] Have date.isocalendar() return a structseq instance

Raymond Hettinger added the comment: It's okay for the pure python equivalent to use collections.namedtuple so long as the C code uses structseq. As Serhiy points out, there is no need to reinvent the wheel. -- ___ Python tracker

[issue24416] Have date.isocalendar() return a structseq instance

Serhiy Storchaka added the comment: namedtuple is much faster now that four years ago. New namedtuple type creation, instantiating a namedtuple object, access to its members -- all this is times faster. It is still slower than tuple in some aspects, because tuples are everywere and the

[issue24416] Have date.isocalendar() return a structseq instance

Change by Raymond Hettinger : -- assignee: rhettinger -> ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe:

[issue24416] Have date.isocalendar() return a structseq instance

Raymond Hettinger added the comment: [Paul Ganssle] > "grab a single component" use is overwhelmingly the common case, > I think it's worth it in the end. Dong-hee Na, you can now go ahead with the patch. The relevant function is date_isocalendar() located in Modules/_datetime.c. Model

[issue24416] Have date.isocalendar() return a structseq instance

Paul Ganssle added the comment: I have compiled both versions with optimizations on, looks like the gap gets a bit smaller (percentage-wise) after that: benchmark| master (ns) | PR 15633 (ns) | Δ (%)

[issue24416] Have date.isocalendar() return a structseq instance

Paul Ganssle added the comment: I haven't had time to try this with an optimized build, I have done a few more benchmarks using a standard build, I'm seeing almost a 50% regression on isocalendar() calls, but the picture is a little rosier if you consider the fact that you need to construct

[issue24416] Have date.isocalendar() return a structseq instance

Change by Tim Peters : -- assignee: -> rhettinger ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe:

[issue24416] Have date.isocalendar() return a structseq instance

Raymond Hettinger added the comment: [Tim Peters] > I favor making this a structseq If there are no further objections to upgrading isocalendar() to return a structseq, I would like to take the issue back so I can supervise Dong-hee Na writing a patch and then help bring it fruition.

[issue24416] Have date.isocalendar() return a structseq instance

Tim Peters added the comment: I favor making this a structseq, primarily based on Paul's attempt to find actual use cases, which showed that named member access would be most useful most often. I have no intuition for that myself, because while I wrote the original functions here, I've

[issue24416] Have date.isocalendar() return a structseq instance

STINNER Victor added the comment: > It seems to affect performance. Oh ok. So this change makes .isocalendar() slower and breaks the backward compatibility on the serialization. I'm not longer sure that it's worth it. I defer the decision the datetime module maintainers: Alexander Beloposky

[issue24416] Have date.isocalendar() return a structseq instance

Dong-hee Na added the comment: @vstinner Here is the new benchmark with you suggested. It seems to affect performance. == Baseline == Mean +- std dev: 134 ns +- 2 ns == PR 15633 == Mean +- std dev: 174 ns +- 5 ns -- ___ Python tracker

[issue24416] Have date.isocalendar() return a structseq instance

STINNER Victor added the comment: """ ./python.exe -m timeit -r11 -s 'import datetime' -s 'a = datetime.datetime.now().isocalendar()' 5000 loops, best of 11: 8.72 nsec per loop Let me know if the benchmark code is not appropriate """ Hum wait, this benchmark measures the performance of

[issue24416] Have date.isocalendar() return a structseq instance

Tal Einat added the comment: > Would it be possible to tell pickle to serialize .isocalendar() as a tuple, > and deserialize it from a tuple to a structseq? The former is possible but that latter is not: If the object is pickled as a tuple, it will always be unpickled as a simple tuple. To

[issue24416] Have date.isocalendar() return a structseq instance

STINNER Victor added the comment: The main blocker issue (if we decide to accept this feature) is the pickle serialization issue. Serhiy Storchaka: > No, it is not fully backwards-compatible. First, if pickle a namedtuple, it > can't be unpickled in previous versions. Would it be possible

[issue24416] Have date.isocalendar() return a structseq instance

Change by Raymond Hettinger : -- priority: normal -> low ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe:

[issue24416] Have date.isocalendar() return a structseq instance

Raymond Hettinger added the comment: Paul, you're a new core dev. Please don't take issues away from the assignee. Please try to learn from more seasoned core devs. Don't argue with everything they say and demand they give you justifications. I asked Tim to opine on this because 1) he is

[issue24416] Have date.isocalendar() return a structseq instance

Paul Ganssle added the comment: In an effort to get a sense of how useful this would actually be, I did a code search for `.isoformat()` on github. I saw a few doctests that will break (if they're even being run) if we make this change, but I also found that the *vast* majority of uses of

[issue24416] Have date.isocalendar() return a structseq instance

Paul Ganssle added the comment: > What IS unprecedented is having a C function bend over backwards to return an > instance of collections.namedtuple(). Is this an issue that anyone is currently insisting upon? From what I can tell the current implementation uses a structseq and none of my

[issue24416] Have date.isocalendar() return a structseq instance

Raymond Hettinger added the comment: What IS unprecedented is having a C function bend over backwards to return an instance of collections.namedtuple(). The only case I know of is with functools.lru_cache() and it was done there because we didn't really have a choice -- there was a

[issue24416] Have date.isocalendar() return a structseq instance

Raymond Hettinger added the comment: FWIW, most things in Python that return a structseq previously returned a tuple (for example, time.localtime() and sys.version_info). This is not an unprecedented upgrade to improve the repr and provide access by field name in addition to positional

[issue24416] Have date.isocalendar() return a structseq instance

Dong-hee Na added the comment: @vstinner == baseline == ./python.exe -m timeit -r11 -s 'import datetime' -s 'a = datetime.datetime.now().isocalendar()' 5000 loops, best of 11: 8.73 nsec per loop == proposed == ./python.exe -m timeit -r11 -s 'import datetime' -s 'a =

[issue24416] Have date.isocalendar() return a structseq instance

STINNER Victor added the comment: > By contrast, returning a plain tuple from `isocalendar()` is the easier *and* > more performant thing to do, and given that any benefits seem marginal I'm > against the switch. Does someone have benchmark numbers? If we go with structseq, creating a new

[issue24416] Have date.isocalendar() return a structseq instance

Paul Ganssle added the comment: > Dong-hee Na, if you want to make a fresh PR for this and bring it to > fruition, I would be happy to review and apply it. It seems premature to say that you will accept a PR for this when there's no consensus for actually adding the feature, and it would be

[issue24416] Have date.isocalendar() return a structseq instance

Raymond Hettinger added the comment: Why close this? Having isocalendar() return a structseq instance would be a nice improvement. It is what structseq was designed for. Dong-hee Na, if you want to make a fresh PR for this and bring it to fruition, I would be happy to review and apply it.