classification
Title: Have date.isocalendar() return a structseq instance
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.9
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: belopolsky, berker.peksag, bmispelon, corona10, p-ganssle, petr.viktorin, rhettinger, serhiy.storchaka, taleinat, tim.peters
Priority: low Keywords: patch

Created on 2015-06-09 12:37 by bmispelon, last changed 2020-05-28 15:47 by petr.viktorin. This issue is now closed.

Files
File name Uploaded Description Edit
issue24416_2.diff bmispelon, 2015-06-09 21:06 review
issue24416_3.diff bmispelon, 2015-06-09 21:32 review
issue24416_4.diff bmispelon, 2015-06-14 10:42 review
issue24416_5.diff bmispelon, 2016-09-20 08:02 review
Pull Requests
URL Status Linked Edit
PR 15633 closed corona10, 2019-09-01 06:32
PR 17651 closed corona10, 2019-12-18 10:13
PR 20113 merged p-ganssle, 2020-05-15 17:10
Messages (53)
msg245061 - (view) Author: Baptiste Mispelon (bmispelon) * Date: 2015-06-09 12:37
Currently, `date.isocalendar()` returns a plain tuple of (year, week, weekday).

It would be a bit more useful if this tuple could be made into a namedtuple (with fields year, week and weekday).
msg245084 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-06-09 19:29
Is there any way that this could break existing code?
msg245085 - (view) Author: Baptiste Mispelon (bmispelon) * Date: 2015-06-09 19:31
As far as I know, using a namedtuple in place of a tuple is fully backwards-compatible.
msg245086 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-06-09 19:36
No, it is not fully backwards-compatible. First, if pickle a namedtuple, it can't be unpickled in previous versions. Second, namedtuple is slower and larger than tuple, so it shouldn't be used in memory or performance critical code.
msg245087 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-06-09 19:51
> First, if pickle a namedtuple, it can't be unpickled in previous versions.

True, but I don't think Python goes as far as to promise that objects pickled in one version can be unpickled in previous versions.

> Second, namedtuple is slower and larger than tuple, so it shouldn't be used in memory or performance critical code.

True, but I doubt that such tuples are often used extensively in performance-critical areas of code. Also, code which does care about this could immediately convert these namedtuples into plain tuples, which would be backwards compatible.
msg245088 - (view) Author: Baptiste Mispelon (bmispelon) * Date: 2015-06-09 19:52
I didn't know about issues with pickling. As for the performance issue, is date.isocalendar() really performance critical?

I found a precedent for replacing a tuple by a namedtuple: https://hg.python.org/cpython/rev/ef72142eb8a2

I'm trying my hand at writing a patch (I'm a new contributor) but it seems the code is implemented twice: both in Lib/ (python) and in Modules/ (C). I'm having a hard time figuring out how to conjure up a namedutple from the C code. Any hepl is appreciated.

Thanks.
msg245089 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-06-09 20:10
You can take a look at lru_cache_cache_info in Modules/_functoolsmodule.c for an example of namedtuple instantiation in C code. But that code gets the namedtuple class as a parameter.

This is not my area of expertise, but you could try using PyObject_CallFunction to call the Python collections.namedtuple function, keep the result as a module attribute, and then call that whenever you want to create an instance.
msg245092 - (view) Author: Baptiste Mispelon (bmispelon) * Date: 2015-06-09 21:06
Here's a second attempt at a patch which seems to work.

I don't know C at all but I used _decimal.c to try and figure out how to use namedtuple in C.

The code compiles and the datetime test suite runs.

I've added a test for the new feature and a note in Misc/NEWS as well as a versionadded note in the relevant documentation.

Let me know if I've missed anything.
msg245093 - (view) Author: Baptiste Mispelon (bmispelon) * Date: 2015-06-09 21:32
Some C code cleanups suggested by haypo.
msg245094 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-06-09 21:35
Now for the color of the bike-shed: What should the namedtuple returned by date.isocalendar() be named? Perhaps CalendarDate or DateTuple?

Baptiste's patches use ISOCalendarResult. In my opinion that is a poor name since it does not tell you anything about what it represents (you have to know or look up what isocalendar() returns).
msg245095 - (view) Author: Baptiste Mispelon (bmispelon) * Date: 2015-06-09 21:38
For the name, I took (un)inspiration from ParseResult: https://docs.python.org/3/library/urllib.parse.html?highlight=urlparse#urllib.parse.ParseResult

Any better suggestion is welcome of course.
msg245115 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-06-10 06:16
How about IsoCalendarDate?
msg245339 - (view) Author: Baptiste Mispelon (bmispelon) * Date: 2015-06-14 10:42
Here's a new patch with the name `IsoCalendarDate` used (it also fixes a wrong version in the `versionadded` section in the docs).

Thanks
msg276835 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2016-09-17 20:07
+1 This looks like a reasonable and useful API improvement.
msg276978 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2016-09-19 13:32
C code should generally use structseq instead of collections.namedtuple.  See the code for time.localtime() in Modules/timemodule.c.

Also, in the docs use "named tuple" as two words and link to the glossary: https://docs.python.org/3/glossary.html#term-named-tuple
msg277016 - (view) Author: Baptiste Mispelon (bmispelon) * Date: 2016-09-20 08:02
I updated the patch based on Raymond's feedback.

I don't know C at all and I tried to mimic the namedtuple usage of timemodule.c as much as I could (until the code compiled and the test suite passed).

I still have two questions:

* It seems that the Python implementation (Lib/datetime.py) is not tested when I run `./python -m test test_datetime` (I introduced a deliberate error in datetime.isocalendar() there but the test still succeeded). Is there a particular procedure to trigger those tests?

* I'm currently using the name `IsoCalendarDate` as suggested by user taleinat. However, shouldn't it be `ISOCalendarDate`?

Thanks.
msg277035 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2016-09-20 15:24
Regarding the name "IsoCalendarDate", see for example this question on Stack Overflow[1] where both of the leading answers suggest beginning with "Iso" or "iso" rather than "ISO". However, taking a look at the relatively new module urllib.request[2], it uses names such as "HTTPHandler" rather than "HttpHandler". I'll leave this up to someone more knowledgeable about the conventions to settle.

.. [1]: http://stackoverflow.com/questions/15526107/acronyms-in-camelcase
.. [2]: https://docs.python.org/3.5/library/urllib.request.html#module-urllib.request
msg350920 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2019-08-31 15:40
@vstinner @taleinat
This patch of this issue is not merged yet.
If there is no reason to abandon this patch,
is it okay for me to continue working on it?
msg350921 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2019-08-31 15:42
This patch/The patch
msg350926 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2019-08-31 18:25
Dong-hee Na, yes, that would be welcome.

Making a PR would be a great first step.

I also suggest adding a note about the issue unpickling with earlier versions, in the NEWS entry and it the "What's New" entry.
msg350951 - (view) Author: Paul Ganssle (p-ganssle) * (Python committer) Date: 2019-09-01 13:51
Sorry for the late response after a patch, but I'm actually -1 on this patch. I don't think it buys us very much in terms of the API considering it only has 3 parameters, and it adds some complexity to the code (in addition to the performance issue). Honestly, I think the main reason we didn't go with positional-only parameters in `fromisocalendar` was that the "/" syntax didn't exist yet.
msg350954 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2019-09-01 14:47
@p-ganssle @taleinat

I agree that there are penalties of this patch what you said.
But I'm wondering how the `fromisocalendar` API relates to this patch.
Rather, wouldn't this patch contribute to improving the usability of the `fromisocalendar` API?
I would appreciate it if you would generously understand that my background knowledge of this module is less than yours.

AS-IS:

>>> year, week, weekday = a.isocalendar()
>>> datetime.date.fromisocalendar(year, week, weekday)
datetime.date(2019, 9, 1)

TO-BE:
>>> b = a.isocalendar()
>>> datetime.date.fromisocalendar(b.year, b.week, b.weekday)
datetime.date(2019, 9, 1)
msg350955 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-09-01 15:19
You can use datetime.date.fromisocalendar(*b).
msg350956 - (view) Author: Paul Ganssle (p-ganssle) * (Python committer) Date: 2019-09-01 16:12
> But I'm wondering how the `fromisocalendar` API relates to this patch.
> Rather, wouldn't this patch contribute to improving the usability of the `fromisocalendar` API?

The `fromisocalendar` API relates to this patch only insofar as it is the inverse function of `isocalendar` and in some sense it allows specifying the parameters by keyword rather by position. I was merely bringing up that we didn't choose that API because we thought people would or should want or need to specify the individual components by keyword but because we didn't have an easy way to maintain the same API in the pure Python and C APIs at the time. 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.

I think that the usability of `fromisoformat` with the output of `isocalendar` will be largely unchanged if we were to switch to returning a namedtuple.
msg350957 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2019-09-01 16:32
@pganssle @serhiy.storchaka

Sounds reasonable on the view of maintaining pure Python and C APIs at the time
Thank you for the explanation.
msg350958 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2019-09-01 16:43
Paul, I'm also on the fence regarding the advantage of this.  It's not clear that the minor possible benefits outweigh the additional complexity and slight backwards-incompatibility (pickling).

Thinking about this further, yes, we'd probably avoid making this change.
msg350959 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2019-09-01 16:50
@taleinat @p-ganssle

I am okay with closing this issue without any change.
My initial purpose for this issue was also whether or not to close a long left issue.

I am +1 to closing this issue without any change.
Please leave a vote for this issue.
If @taleinat and @p-ganssle agree with closing this issue.
I am going to close this issue!

Thanks always!
msg350960 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2019-09-01 20:05
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.

I'm changing the title to structseq.  It was a distractor to mention collections.namedtuple() at all -- that would have been more appropriate for pure python code.
msg350968 - (view) Author: Paul Ganssle (p-ganssle) * (Python committer) Date: 2019-09-02 02:04
> 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 good to probably work out if it's even desirable before asking contributors to do more work on it.

It seems like it would be better to argue the point of *why* you think a structseq actually solves the problem here. Is a struct sequence more backwards compatible than a namedtuple? Less? Is it as performant? Will it make it easier or harder to maintain compatibility between the C and pure Python implementations of the datetime module?
msg350979 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-09-02 10:52
> 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 instance should be almost as efficient as creating a tuple, no?
msg350986 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2019-09-02 11:31
@vstinner

== baseline ==
./python.exe -m timeit -r11 -s 'import datetime' -s 'a = datetime.datetime.now().isocalendar()'
50000000 loops, best of 11: 8.73 nsec per loop

== proposed ==
./python.exe -m timeit -r11 -s 'import datetime' -s 'a = datetime.datetime.now().isocalendar()'
50000000 loops, best of 11: 8.72 nsec per loop

Let me know if the benchmark code is not appropriate
msg351045 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2019-09-03 00:33
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 access.
msg351046 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2019-09-03 01:04
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 pre-existing pure python API that already used collections.namedtuple().

ISTM the cross-version pickling issue is minor red herring.  We've cheerfully upgraded tuples to structseqs on a number of occasions and it hasn't been an issue.

Tim, would you please weigh in on this so we can put this to bed, either closing the request because we're too meek to make any change, 
upgrading to structseq to provide the requested functionality, or twisting our code in weird ways to have a C function become dependent on a pure python module.
msg351087 - (view) Author: Paul Ganssle (p-ganssle) * (Python committer) Date: 2019-09-03 15:06
> 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 objections had to do with the properties of a structseq.

> ISTM the cross-version pickling issue is minor red herring.  We've cheerfully upgraded tuples to structseqs on a number of occasions and it hasn't been an issue.

I generally agree with this - it is nice to not break this compatibility when there's no good reason to do so, but pickling data in one version of Python and unpickling it in another is not something that's supported by the pickle module anyway.

> Tim, would you please weigh in on this so we can put this to bed, either closing the request because we're too meek to make any change, 
upgrading to structseq to provide the requested functionality, or twisting our code in weird ways to have a C function become dependent on a pure python module.

I must take some umbrage at this framing of the question. I don't even know where meekness comes into it - adding *any* new functionality brings support burdens and additional code complexity and changes to code that's been stable for a long time like `dateutil.isocalendar` is particularly likely to cause breakages if only because of the time people have had to start relying on the specific implementation. I have merely asked for a justification and an argument other than your subjective judgement that this is a nice improvement.
msg351093 - (view) Author: Paul Ganssle (p-ganssle) * (Python committer) Date: 2019-09-03 18:15
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 `isocalendar` seem to be people pulling out a single component of it, like:  `return datetime.datetime.now().isocalendar()[1]`.

That is not the kind of usage pattern I was envisioning when I said that this was a marginal improvement, a *lot* of this code could be made considerably more readable with named fields. If indeed the performance is similar or the same and this won't impact consumers of the pure python version of the module unduly (I checked in #pypy and they think that it shouldn't be more than a minor irritation if anything), then I am changing my -1 to a +1.
msg351109 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2019-09-04 02:25
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 one of the principal authors of the datetime module, 2) he was party to previous decisions to upgrade from tuples to struct seq, 3) he has a good deal of common sense and knows what is important, and 4) he's far more senior than me, so there is a chance you might listen to him (my comments don't seem to carry any weight with you).
msg351122 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-09-04 12:28
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 to tell pickle to serialize .isocalendar() as a tuple, and deserialize it from a tuple to a structseq? I'm thinking at __getstate__ and __setstate__ methods, but I'm not sure how to use them.


Serhiy Storchaka:
> Second, namedtuple is slower and larger than tuple, so it shouldn't be used in memory or performance critical code.

vstinner@apu$ python3
Python 3.7.4 (default, Jul  9 2019, 16:32:37) 
>>> import sys
>>> s=sys.version_info
>>> t=tuple(s)
>>> sys.getsizeof(t), sys.getsizeof(s)
(96, 96)
>>> type(s)
<class 'sys.version_info'>

Hum, structseq and tuple seem to have exactly the same memory footprint: only the type is larger, not instances.

According to msg350986, the performance to instanciate a new object is exactly the same between tuple and structseq.
msg351178 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2019-09-05 06:00
> 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 customize unpickling, the serialized data must include the name of the class to use, and that class will never exist in earlier Python versions. I don't think there's a way around this.

However, I find Raymond's note very convincing, in that we should likely not let the unpickling issue get in the way of this improvement:

> 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 access.
msg351197 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-09-05 11:16
"""
./python.exe -m timeit -r11 -s 'import datetime' -s 'a = datetime.datetime.now().isocalendar()'
50000000 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 the pass opcode... That's why it's so fast :-) -s 'a = datetime.datetime.now().isocalendar()' is run exactly once for the whole benchmark, not at each benchmark iteration...

I suggest you this microbenchmark instead:

./python.exe -m pyperf timeit -s 'import datetime; dt = datetime.datetime.now()' 'dt.isocalendar()'

For example, on my Fedora 30 with Python 3.7.4, I get:

Mean +- std dev: 108 ns +- 4 ns
msg351198 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2019-09-05 13:26
@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
msg351252 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-09-06 13:51
> 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 and Paul Ganssle.
msg351280 - (view) Author: Tim Peters (tim.peters) * (Python committer) Date: 2019-09-07 05:21
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 never used them and never will ;-)  But if I ever did, I'd probably want the ISO week number, and would appreciate not having to remember which meaningless index means "week".

Don't care about moving pickles to older versions.

Don't care about the putative speed hit - it's still measured in nanoseconds, so can still create over a million per second.  In apps with a large number of dates, they're typically not _calculated_, but read up from a relatively snail-like database or network connection.  I doubt anyone would notice if `.isocalendar()` got 10x slower.

Note:  I'm unassigning myself, because I have nothing else to say :-)
msg351314 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2019-09-08 01:41
[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.
msg351327 - (view) Author: Paul Ganssle (p-ganssle) * (Python committer) Date: 2019-09-08 11:37
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 a `date` or `datetime` object in the first place and anyone concerned with performance will likely be making exactly one `isocalendar()` call per datetime object. The common case is that they construct the datetime with a datetime literal or something equivalent. The "worst case scenario" is that they construct a single "seed" datetime and then construct many new datetimes with faster operations like adding a timedelta.

I have compared the following cases:

call only: -s 'import datetime; dt = datetime.datetime(2019, 1, 1)' 'dt.isocalendar()'
constructor + call: -s 'import datetime' 'dt = datetime.datetime(2019, 1, 1); dt.isocalendar()'
timedelta + call: -s 'import datetime; dt = datetime.datetime(2019, 1, 1); td = timedelta(days=1)' '(dt + td).isocalendar()'


The results are summarized below, the most likely real practical impact on performance-sensitive "hot loops" would be a 29% performance regression *if* isocalendar() is the bottleneck:


       benchmark                    |  master (ns)  |  PR 15633 (ns)  |  Δ (%)
------------------------------------+---------------+-----------------+----------
call only (datetime)                |    349 (±14)  |     511 (±22)   |   46
constructor + call (datetime)       |    989 (±48)  |    1130 (±50)   |   14
timedelta + call (datetime)         |    550 (±14)  |     711 (±22)   |   29


The numbers for `datetime.date` are basically similar:

       benchmark                    |  master (ns)  |  PR 15633 (ns)  |  Δ (%)
------------------------------------+---------------+-----------------+----------
call only (date)                    |    360 (±18)  |     530 (±41)   |   47
constructor + call (date)           |    824 (±17)  |     975 (±29)   |   18
timedelta + call (datetime)         |    534 (±20)  |     685 (±24)   |   28


This is a bit disheartening, but I think the fact that these performance sensitive situations are rare and the "grab a single component" use is overwhelmingly the common case, I think it's worth it in the end.

If there are significant complaints about the performance regression, we may be able to create a variant method similar to the way that `chain` has `chain.from_iterables`, something like `dt.isocalendar.as_tuple()` for the performance-sensitive folks. That said, that's very YAGNI, we should not do that unless someone comes forward with a real use case that will be adversely affected here.
msg351335 - (view) Author: Paul Ganssle (p-ganssle) * (Python committer) Date: 2019-09-08 18:16
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)  |  Δ (%)
------------------------------------+---------------+-----------------+----------
call only (datetime)                |     73 (±3)   |     92.3 (±7)   |   26
constructor + call (datetime)       |    228 (±9)   |     260 (±16)   |   14
timedelta + call (datetime)         |    108 (±5)   |     128 (±9)    |   18

If this were something fundamental like a performance regression in building a tuple or constructing a dictionary or something I'd be concerned, but this just reinforces my feeling that, on balance, this is worth it, and that we are probably not going to need a "fast path" version of this.
msg351339 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2019-09-08 22:32
[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 the structseq code after the PyFloat_GetInfo() function in Objects/floatobject.h.

When you have a PR with a doc update and tests, please request my review and I help you wrap it up.

Ask questions here if you get stuck.  Good luck!
msg352244 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-09-13 07:57
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 interpreter has special optimizations for tuples. But date.isocalendar() is not used in such performance critical code.

The size of pickles will grow, the time of pickling and unpickling will increase, and old Python versions will not be able to unpickle a new type by default. But I do not think that instances of this type will be pickled in large numbers so it will has a global effect, and the problem with compatibility can be solved with some hacks. If anybody pickles them at all.

It was not clear whether adding a new type will give any benefit in real code. Thanks to Paul for his investigations.

The only thing that makes this case unique is that we have two implementations of the datetime module. Therefore we will need either add two implementation of a new named tuple type (which are different in details) or import the Python implementation into the C code. This increases the cost of the implementation and maintaining.
msg352373 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2019-09-13 17:12
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.
msg352377 - (view) Author: Tim Peters (tim.peters) * (Python committer) Date: 2019-09-13 17:26
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 docs to say that this function returns a "named tuple" (the docs are now clear that there may be more than one implementation of such a thing, and that the only things you can _rely_ on are that they support named element access in addition to tuple methods).
msg352381 - (view) Author: Paul Ganssle (p-ganssle) * (Python committer) Date: 2019-09-13 17:33
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 any more complicated than what we did we probably would have just gone with a namedtuple.

The only thing that's holding things up now is that we're working out a way to maintain the ability to pickle the object without making the class public. This is not really a hard requirement, but I'd like to give it an honest effort before calling it a day and just relying on "it's not in __all__, therefore it's not public." (I should note that we can only take that approach after issue #38155 is resolved, which is another reason for the delay).

In any case, the bulk of the conversation on the implementation has been taking place on GH-15633, sorry for the split discussion location, folks.
msg369046 - (view) Author: Paul Ganssle (p-ganssle) * (Python committer) Date: 2020-05-16 14:03
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
msg369047 - (view) Author: Paul Ganssle (p-ganssle) * (Python committer) Date: 2020-05-16 14:17
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 I suggested it, I recognize that this is a /weird thing to do/, and I'm sorta banking on the fact that in all likelihood no one is relying on pickling and unpickling the result of .isocalendar() returning anything but a tuple (since, currently, that's already what it does, regardless of what the input type was).

I suppose we'll have to see if it causes problems in the beta period, and I'll keep a close eye on it.
msg370231 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2020-05-28 15:47
This broke compilation with mingw; see https://bugs.python.org/issue40777
History
Date User Action Args
2020-05-28 15:47:12petr.viktorinsetnosy: + petr.viktorin
messages: + msg370231
2020-05-16 14:17:08p-gansslesetstatus: open -> closed
resolution: fixed
messages: + msg369047

stage: patch review -> resolved
2020-05-16 14:03:17p-gansslesetmessages: + msg369046
2020-05-15 17:10:52p-gansslesetpull_requests: + pull_request19420
2020-01-15 21:32:47vstinnersetnosy: - vstinner
2019-12-18 10:13:22corona10setpull_requests: + pull_request17118
2019-09-13 17:33:43p-gansslesetmessages: + msg352381
2019-09-13 17:26:11tim.peterssetmessages: + msg352377
2019-09-13 17:12:39rhettingersetmessages: + msg352373
2019-09-13 07:57:27serhiy.storchakasetmessages: + msg352244
2019-09-13 06:03:04rhettingersetassignee: rhettinger ->
2019-09-08 22:32:33rhettingersetmessages: + msg351339
2019-09-08 18:16:47p-gansslesetmessages: + msg351335
2019-09-08 11:37:44p-gansslesetmessages: + msg351327
2019-09-08 02:01:21tim.peterssetassignee: rhettinger
2019-09-08 01:41:24rhettingersetmessages: + msg351314
2019-09-07 05:21:03tim.peterssetassignee: tim.peters -> (no value)
messages: + msg351280
2019-09-06 13:51:47vstinnersetmessages: + msg351252
2019-09-05 13:26:46corona10setmessages: + msg351198
2019-09-05 11:16:51vstinnersetmessages: + msg351197
2019-09-05 06:00:06taleinatsetmessages: + msg351178
2019-09-04 12:28:22vstinnersetmessages: + msg351122
2019-09-04 02:25:46rhettingersetpriority: normal -> low
2019-09-04 02:25:04rhettingersetassignee: p-ganssle -> tim.peters
messages: + msg351109
2019-09-03 18:15:10p-gansslesetassignee: tim.peters -> p-ganssle
messages: + msg351093
2019-09-03 15:06:22p-gansslesetmessages: + msg351087
2019-09-03 01:04:27rhettingersetassignee: rhettinger -> tim.peters
messages: + msg351046
2019-09-03 00:33:26rhettingersetnosy: + tim.peters
messages: + msg351045
2019-09-02 11:31:52corona10setmessages: + msg350986
2019-09-02 10:52:44vstinnersetmessages: + msg350979
2019-09-02 02:04:49p-gansslesetmessages: + msg350968
2019-09-01 20:05:01rhettingersetassignee: belopolsky -> rhettinger
messages: + msg350960
title: Return a namedtuple from date.isocalendar() -> Have date.isocalendar() return a structseq instance
2019-09-01 16:50:36corona10setmessages: + msg350959
2019-09-01 16:43:12taleinatsetmessages: + msg350958
2019-09-01 16:32:56corona10setmessages: + msg350957
2019-09-01 16:12:33p-gansslesetmessages: + msg350956
2019-09-01 15:19:11serhiy.storchakasetmessages: + msg350955
2019-09-01 14:47:09corona10setmessages: + msg350954
2019-09-01 13:51:42p-gansslesetnosy: + p-ganssle
messages: + msg350951
2019-09-01 06:32:10corona10setstage: needs patch -> patch review
pull_requests: + pull_request15301
2019-08-31 18:25:40taleinatsetmessages: + msg350926
versions: + Python 3.9, - Python 3.7
2019-08-31 15:42:08corona10setmessages: + msg350921
2019-08-31 15:40:29corona10setnosy: + vstinner, corona10
messages: + msg350920
2016-09-20 15:24:59taleinatsetmessages: + msg277035
2016-09-20 08:02:26bmispelonsetfiles: + issue24416_5.diff

messages: + msg277016
2016-09-19 13:32:16rhettingersetmessages: + msg276978
2016-09-17 20:07:35rhettingersetnosy: + rhettinger
messages: + msg276835
2016-09-16 18:57:08belopolskysetassignee: belopolsky

nosy: + belopolsky
versions: + Python 3.7, - Python 3.6
2015-06-14 10:42:58bmispelonsetfiles: + issue24416_4.diff

messages: + msg245339
2015-06-10 06:16:39taleinatsetmessages: + msg245115
2015-06-09 21:38:56bmispelonsetmessages: + msg245095
2015-06-09 21:35:32taleinatsetmessages: + msg245094
2015-06-09 21:32:48bmispelonsetfiles: + issue24416_3.diff

messages: + msg245093
2015-06-09 21:06:36bmispelonsetfiles: + issue24416_2.diff
keywords: + patch
messages: + msg245092
2015-06-09 20:10:39taleinatsetmessages: + msg245089
2015-06-09 19:52:40bmispelonsetmessages: + msg245088
2015-06-09 19:51:36taleinatsetmessages: + msg245087
2015-06-09 19:36:33serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg245086
2015-06-09 19:31:12bmispelonsetmessages: + msg245085
2015-06-09 19:29:06taleinatsetnosy: + taleinat
messages: + msg245084
2015-06-09 15:04:56berker.peksagsetnosy: + berker.peksag

type: enhancement
stage: needs patch
2015-06-09 12:37:20bmispeloncreate