python / peps

Python Enhancement Proposals
https://peps.python.org
4.36k stars 1.51k forks source link

PEP 495 contains incorrect diagram for Mind the Gap section #2959

Closed bxparks closed 1 year ago

bxparks commented 1 year ago

In PEP 495, the diagram in the Mind the Gap section is the same as the diagram in the In the Fold section just above it. I believe this is incorrect. The correct diagram should look like the following. I tried to reproduce the font and color scheme, but it's not perfect:

2023-01-12 PEP495 InGap Diagram 2

bxparks commented 1 year ago

Alternatively, maybe use this diagram which more complicated, but is closer to what the code does when the imaginary datetime in the gap is normalized into an actual time:

tz = zoneinfo.ZoneInfo('America/Los_Angeles')

# In the gap, fold=0 selects the first transition extended forward
t = datetime(2022, 3, 13, 2, 29, 0, tzinfo=tz, fold=0)
u0 = t.timestamp()
t0 = datetime.fromtimestamp(u0, tz=tz)
print(t0) # 2022-03-13 03:29:00-07:00

# In the gap, fold=1 selects the second transition extended backward
t = datetime(2022, 3, 13, 2, 29, 0, tzinfo=tz, fold=1)
u1 = t.timestamp()
t1 = datetime.fromtimestamp(u1, tz=tz)
print(t1) # 2022-03-13 01:29:00-08:00

2023-01-12 PEP495 InGap Diagram 3

CAM-Gerlach commented 1 year ago

Your interpretation would seem to be correct, at least as far as I can tell as a PEP editor.

However, as Final PEPs are generally considered historical records rather than living documentation or standards, and this PEP in particular was originally accepted 7 years ago for a Python version released 6 years ago and no longer officially supported, I'm not sure it's worth the effort to update. However, that's ultimately up to the authors @tim-one and @abalkin , though AFAIK neither of them are very active anymore these days.

bxparks commented 1 year ago

I think all Python versions >= 3.6 implement the fold parameter and follow the algorithms described in PEP 495.

I understand the reluctance to change stable documents. Can PEP documents be appended with an Errata section?

CAM-Gerlach commented 1 year ago

Thanks for understanding. There's no real precedent for that, especially for something that is just illustrative background rather than being a normative, substantive part of the specification—either it is fixed if worth fixing, or left alone. If the PEP is to be updated with a new image, then the incorrect image may as well just be replaced directly.

In general (with some existing exceptions), once a Standards Track PEP is Final, the canonical documentation or specification for it, if needed, should typically be hosted somewhere more appropriate (such as the Python docs) and updated there. The closest canonical place here AFAIK would be the datetime module docs, where the important parts are documented and the general concepts/background justifying the change (like these cartoons) are not included.

But again, with all that said, it's ultimately up to the PEP authors if they think it's worth fixing or not, presuming you'd be willing to submit a suitable replacement (in SVG format, like the originals) in a PR.

bxparks commented 1 year ago

The diagrams that I attached to this ticket are SVG files. If they are satisfactory, I can create a PR to replace https://github.com/python/peps/blob/main/pep-0495-gap.svg with one of those. Either of the 2 above.

Oh, I found the original commit that added the incorrect Gap diagram pep-0495-gap.svg. It looks like initially there were both PNG and SVG versions of these diagrams. The PNG version of the Gap diagram looks very close to my diagrams above. However, the SVG version of the Gap diagram in that commit (pep-0495-gap.svg) seems to be a duplicate of the Fold diagram (pep-0495-fold.png), which is clearly a mistake. I assume the PNG files were then deleted with a later commit. So that's how we got here with the incorrect diagram.

The reason that I am motivated to fix this is because the behavior of the datetime library (or any timezone library in any language) around the Gap is non-trivial, and it took me a long time (1-2 years) to understand all the nuances fully. The incorrect diagram in the PEP did not help. I would like to save people who come after me some grief.

Rosuav commented 1 year ago

This sounds like a bug fix, and I am in favour of fixing bugs. I for one would welcome that PR.

bxparks commented 1 year ago

Here is my reproduction of the correct Gap SVG file that was lost in the original commit. If this is acceptable, I can send a PR to replace the current pep-0495-gap.svg:

2023-01-12 PEP495 InGap Diagram 4

The reproduction is not perfect:

Rosuav commented 1 year ago

LGTM but I'm not a content expert.

CAM-Gerlach commented 1 year ago

LGTM, I'm also not a SME, but the background I do have in this area as well as the fact that it matches the original PNG seems to confirm the change is correct. I'd propose you go ahead your PR, and if Tim and Alexander don't object after a week or two, we can merge it.

I found the original commit that added the incorrect Gap diagram pep-0495-gap.svg. It looks like initially there were both PNG and SVG versions of these diagrams.

Ah, I was wondering if that was the case when we moved to the SVG version being displayed relatively recently, which is why it was never noticed at the time, but hadn't actually gone back to check yet. That makes a particularly strong and compelling case for accepting it, since not only is it a bug fix to the PEP itself, but is fixing a technical regression from how the PEP was originally displayed at the time of its acceptance.

The reason that I am motivated to fix this is because the behavior of the datetime library (or any timezone library in any language) around the Gap is non-trivial, and it took me a long time (1-2 years) to understand all the nuances fully. The incorrect diagram in the PEP did not help. I would like to save people who come after me some grief.

If text in the PEP was important to your understanding of the behavior of the datetime module, then perhaps it would be worth porting the relevant missing portions of the PEP's explanation to the Python docs, as an explanation-type section. Currently the docs cover the (API) "reference" topic thoroughly, but are somewhat more sparse in serving the other areas, particularly explanation. As the goal of explanation-type docs content is to serve understanding of the thing documented, this would seem like a good fit. if you're interested in something like that, I could help guide you through it (especially since I'm also on the Python Docs team).

bxparks commented 1 year ago

@CAM-Gerlach : Sent off a PR. With regards to the Python docs, are you referring to https://docs.python.org/3/library/datetime.html ? That is a beast of a page, I'd be reluctant to touch that one. But one obvious improvement might be to actually add a link to the PEP 495 document near the first occurrence of the fold parameter. Currently there is a single reference to PEP 495, buried in a comment, inside a code fragment, about 2/3 of the way down. Can you point me to the repo that contains the source text for this page? I spent a few minutes searching for it, but got lost in an exploding graph of hyperlinks.

CAM-Gerlach commented 1 year ago

Well, ideally we want the docs to avoid having to reference PEPs to explain things as far as the use of the current implementation goes, as opposed to the historical design decisions that shaped the eventual implementation in CPython.

That said, if you really want, it's the https://github.com/python/cpython repo along with the rest of the source code. You can find a link to the source code of every docs page at the top of said page:

image

The link for this particular page is https://github.com/python/cpython/tree/main/Lib/datetime.py

bxparks commented 1 year ago

Thanks. The link at the top of the datetime docs page points to the source code for datetime.py, not the source code for datetime.rst. The source code for the docs page seems to be at cpython/Docs/library/datetime.rst.

My opinion is that an average software developer would find it very difficult to understand the fold parameter in the datetime library without reading PEP 495, with the correct Gap diagram. During an overlap (e.g. 02:00->01:00 shift in the fall-back), the fold can be hand-wave explained, as currently done in the datetime.rst page. But during a gap (e.g. 02:00->03:00 spring-forward), an accurate explanation of fold in the datetime docs page would need PEP 495.

An extreme example of such a gap is something like the Pacific/Apia time zone in Samoa, which has a 24 hour gap in Dec 2011, going from 2011-12-29 23:59:59 to 2011-12-31 00:00:00, skipping Dec 30 entirely. I think it would be very difficult to explain how the fold parameter handles this case without PEP 495.

In any case, let's tackle this one step at a time:

1) Let's get the Gap diagram fixed with PR #2965. 2) I can review the datetime.rst page on my spare time to see what improvements can be made. But if it is a hard rule that PEP 495 cannot be referenced on that page, then I would probably decline to take this task, because I don't think I can explain fold in a gap without referencing PEP 495, or copying substantial chunks of it.

CAM-Gerlach commented 1 year ago

Thanks. The link at the top of the datetime docs page points to the source code for datetime.py, not the source code for datetime.rst. The source code for the docs page seems to be at cpython/Docs/library/datetime.rst.

Oops, I somehow missed that—my mistake, sorry!

My opinion is that an average software developer would find it very difficult to understand the fold parameter in the datetime library without reading PEP 495, with the correct Gap diagram.

Yup, that would definitely seem to be the case from what you've stated and what you've seen on the page. It gives only very basic API Reference information, and lacks any real explaination-type material discussing the theory and background behind it to actually explain to the reader how it works.

In any case, let's tackle this one step at a time

Good plan :+1:

Let's get the Gap diagram fixed with PR

:+1:

  1. But if it is a hard rule that PEP 495 cannot be referenced on that page, then I would probably decline to take this task, because I don't think I can explain fold in a gap without referencing PEP 495, or copying substantial chunks of it.

It's certainly not any sort of hard rule; PEPs are referenced a number of places to explain the motivation, rationale and history of a specific feature, and it's better to reference the PEP than to simply leave the reader hanging completely when it comes to explanation-type content serving their understanding.

What I was suggesting, rather, is that while adding references to the PEP (:pep:`495`) is okay as a short term stopgap, what would be really helpful is if you could help migrate the relevant existing portions of the PEP that are important to reader understanding of the current fold behavior (as opposed to, e.g., justifying the changes, outlining backward compatibility, outlining alternatives, or specifying the API implementation) to a new top-level explanation section in the datetime docs on the topic.

Since you were in that position yourself as a reader, your insight as to what parts of the PEP were most useful, and how to best present them in a reader-friendly manner in the docs, would be uniquely valuable here and we'd be happy to help guide you in this.

abalkin commented 1 year ago

I created the gap sketch with Inkscape and the source code was committed to the hg.python.org repo which is still available. I will try to extract the original file an post it here, but from the diff, I see that there was an exporting problem:

+   inkscape:version="0.91 r13725"
+   sodipodi:docname="pep-0495-gap.svg"
+   inkscape:export-filename="/Users/a/Work/peps/pep-0495-fold.png"
abalkin commented 1 year ago

Interestingly, the PNG versions look right in the mercurial repo

Fold Gap
pep-0495-fold-2 pep-0495-gap

But the SVG ones are messed up

Fold (svg) Gap (svg)
pep-0495-fold pep-0495-gap

I assume the original version of the PEP was published with PNG images and that's why my mistake posting SVG sources went unnoticed for so long.

I will try to find the correct originals, but it may be hard after 7 years. :)

CAM-Gerlach commented 1 year ago

Yup, that's exactly the same thing as here; the problem was missed in the 7 years since until relatively recently, when the source for the PEP rendering was switched over to the SVG version from the PNG one (since the legacy rendering system we got rid of a year ago didn't support modern SVG).

Thanks, let us know! If you can't or its too much trouble, we can always just go ahead with #2965 as is as a backup plan.

abalkin commented 1 year ago

The #2965 version is indeed very close to the original. Thanks, @bxparks !

The only issue that I noticed was rendering of the subscripts on the $u_1$ and $u_0$ labels.

bxparks commented 1 year ago

@abalkin : Google Draw renders its subscript significantly below the normal text, see below. The closest I could get to the original was to use 14pt font for the u and 10pt font for the 0 and 1. If you can find the correct SVG image for the gap, that would be great.

Thanks for the work on PEP 495 by the way! Of all the different ways of handling local time overlaps and gaps that I have surveyed (C++ HowardHinnant/date, C# Noda Time, Java java.time, Java Joda-Time, Go time), PEP 495 is the most reasonable and flexible, especially when implementing a timezone library in a language or environment that does not support exceptions.

2023-01-12 PEP495 InGap Diagram 4 (subscript)

abalkin commented 1 year ago

It appears that I only saved latest PNG versions and not the SVG source. I have to admit that it was my first time using Inkscape. Let's go with the versions in the PR for now - it is important to get this fixed. I will try to find time to add a section to the official datetime documentation hopefully with better text and graphics than in the PEP.

CAM-Gerlach commented 1 year ago

Thanks @abalkin — I'll go ahead and merge that, then :+1: