GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-109218: Improve documentation for the complex() constructor by serhiy-storchaka · Pull Request #119687 · python/cpython

Conversation

@serhiy-storchaka

@serhiy-storchaka serhiy-storchaka commented May 28, 2024 •
edited by github-actions Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • Issue: Invalid "equivalents" of the complex type constructor in docs #109218

📚 Documentation preview 📚: https://cpython-previews--119687.org.readthedocs.build/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@serhiy-storchaka
pythongh-109218: Improve documentation for the complex() constructor
e67380d
@serhiy-storchaka serhiy-storchaka added docs Documentation in the Doc dir skip news needs backport to 3.12 only security fixes needs backport to 3.13 only security fixes labels May 28, 2024
@serhiy-storchaka
serhiy-storchaka requested a review from mdickinson May 28, 2024 20:14
@bedevere-app bedevere-app Bot added the awaiting core review label May 28, 2024
@bedevere-app bedevere-app Bot mentioned this pull request May 28, 2024
Invalid "equivalents" of the complex type constructor in docs #109218
Closed
skirpichev
skirpichev approved these changes May 29, 2024
View reviewed changes

@skirpichev skirpichev left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Few remarks:
0) This touch also bool, float and int constructors; changes looks fine, but at least the commit message could be more verbose and mention that.

  1. Should be merged before #119620
  2. cmath has yet another instance of z == z.real + z.imag*1j invariant, mentioned in the issue thread. I think this should be fixed too, probably just by removing that line.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@serhiy-storchaka
Fix also cmath.rst.
94deb98
AlexWaygood
AlexWaygood reviewed May 29, 2024
View reviewed changes
Comment thread Doc/library/functions.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/functions.rst Outdated
Comment on lines +382 to +392
If the argument is a string, it should contain a decimal number, optionally
preceded by a sign, and optionally followed by the ``j`` or ``J`` suffix,
or a pair of decimal numbers, optionally preceded by a sign, separated by
the ``+`` or ``-`` operator, and followed by the ``j`` or ``J`` suffix.
A string representing a NaN (not-a-number), or infinity is acceptable
in place of a decimal number, like in :func:`float`.
The string can optionally be surrounded by whitespaces and the round
parenthesis ``(`` and ``)``, which are ignored.
The string must not contain whitespace between ``+``, ``-``, the ``j`` or
``J`` suffix, and the decimal number. For example, ``complex('1+2j')`` is fine, but
``complex('1 + 2j')`` raises :exc:`ValueError`.

@AlexWaygood AlexWaygood May 29, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a lot of detail for the second paragraph. Users might find it easier to understand how to create complex numbers if the REPL examples came before this full specification

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@serhiy-storchaka serhiy-storchaka May 29, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I followed the example of float() which has even more detailed description of the input format in the second paragraph. I merged it with a note about whitespaces, so all information related to parsing string to complex is now in one place.

I would be happy to make the description shorter, even if less detailed. Do you have any suggestions?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@AlexWaygood AlexWaygood May 29, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, though I would honestly also be inclined to move the float() examples higher up as well. I think it's probably correct to provide a full specification here; I just worry that it could overwhelmingly technical for Python beginners, who will probably learn best by looking at the examples.

In any event, I don't have a strong opinion; I'm happy to defer to you and Mark

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@mdickinson mdickinson May 29, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @AlexWaygood has a good point here: for readability, it would be nice to see just three examples (one for each of the three signatures) following the initial "Convert a single string or number to a complex number, or ..." line, before we get into the heavy detail of exactly which strings are permitted.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@serhiy-storchaka serhiy-storchaka May 29, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have moved examples for complex() and float() and added examples for int().

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Doc/library/functions.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka @AlexWaygood
Apply suggestions from code review
7182597
Co-authored-by: Alex Waygood <[email protected]>
mdickinson
mdickinson approved these changes May 29, 2024
View reviewed changes

@mdickinson mdickinson left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

This is very much a judgement call and personal opinion, and I don't know how well it fits with the general direction of our documentation, but I do think it would be nice to have a small number of examples immediately following the initial "Convert a single string ..." doc line.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Doc/library/functions.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/functions.rst Outdated
Comment on lines +382 to +392
If the argument is a string, it should contain a decimal number, optionally
preceded by a sign, and optionally followed by the ``j`` or ``J`` suffix,
or a pair of decimal numbers, optionally preceded by a sign, separated by
the ``+`` or ``-`` operator, and followed by the ``j`` or ``J`` suffix.
A string representing a NaN (not-a-number), or infinity is acceptable
in place of a decimal number, like in :func:`float`.
The string can optionally be surrounded by whitespaces and the round
parenthesis ``(`` and ``)``, which are ignored.
The string must not contain whitespace between ``+``, ``-``, the ``j`` or
``J`` suffix, and the decimal number. For example, ``complex('1+2j')`` is fine, but
``complex('1 + 2j')`` raises :exc:`ValueError`.

@mdickinson mdickinson May 29, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @AlexWaygood has a good point here: for readability, it would be nice to see just three examples (one for each of the three signatures) following the initial "Convert a single string or number to a complex number, or ..." line, before we get into the heavy detail of exactly which strings are permitted.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Doc/library/functions.rst
Comment on lines +407 to +409
If both arguments are complex numbers, return a complex number with the real
component ``real.real-imag.imag`` and the imaginary component
``real.imag+imag.real``.

@mdickinson mdickinson May 29, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably this will mention the deprecation if/when #119620 is merged? Or is the plan to remove the mention of allowing complex altogether?

I'd be tempted to not even document this (mis)feature, on the basis that people who already know about it already know about it, and people who don't know about it shouldn't start using it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@serhiy-storchaka serhiy-storchaka May 29, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plan was to leave this until the end of the deprecation period.

I would be happy to remove this, but then we will receive another reports about wrong documentation or behavior. The old real + imag*1j was more correct for input with finite components (and ignoring corner cases with negative zero).

"Each argument may be any numeric type (including complex)." is already in the current documentation, so it is late to hide this without also changing the behavior.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@mdickinson mdickinson May 29, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@skirpichev skirpichev May 30, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Best we can do is to put the deprecation note as close to this sentence, as possible:)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Doc/library/functions.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels May 29, 2024
@serhiy-storchaka
Move examples for complex() and float(). Add examples for int().
63ebe03
AlexWaygood
AlexWaygood approved these changes May 29, 2024
View reviewed changes
Comment thread Doc/library/functions.rst
Comment on lines +386 to +401
>>> complex('+1.23')
(1.23+0j)
>>> complex('-4.5j')
-4.5j
>>> complex('-1.23+4.5j')
(-1.23+4.5j)
>>> complex('\t( -1.23+4.5J )\n')
(-1.23+4.5j)
>>> complex('-Infinity+NaNj')
(-inf+nanj)
>>> complex(1.23)
(1.23+0j)
>>> complex(imag=-4.5)
-4.5j
>>> complex(-1.23, 4.5)
(-1.23+4.5j)

@AlexWaygood AlexWaygood May 29, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest we reorder some of these so that the stranger ones come below the more common ones:

Suggested change
>>> complex('+1.23')
(1.23+0j)
>>> complex('-4.5j')
-4.5j
>>> complex('-1.23+4.5j')
(-1.23+4.5j)
>>> complex('\t( -1.23+4.5J )\n')
(-1.23+4.5j)
>>> complex('-Infinity+NaNj')
(-inf+nanj)
>>> complex(1.23)
(1.23+0j)
>>> complex(imag=-4.5)
-4.5j
>>> complex(-1.23, 4.5)
(-1.23+4.5j)
>>> complex('+1.23')
(1.23+0j)
>>> complex('-4.5j')
-4.5j
>>> complex('-1.23+4.5j')
(-1.23+4.5j)
>>> complex(-1.23, 4.5)
(-1.23+4.5j)
>>> complex(1.23)
(1.23+0j)
>>> complex(imag=-4.5)
-4.5j
>>> complex('\t( -1.23+4.5J )\n')
(-1.23+4.5j)
>>> complex('-Infinity+NaNj')
(-inf+nanj)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@serhiy-storchaka serhiy-storchaka May 29, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this mixes examples for three fundamentally different roles: string parsing, numerical conversion and construction from components.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ghost May 30, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wasn't immediately obvious to me that that was the reason behind your ordering, and I doubt it would be obvious to beginners either, which is why I suggested a different order of "most useful to least useful". But again, I don't have a strong opinion; your PR is certainly fine as-is :-)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ghost May 30, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also expect the most useful to the least useful examples even though it mixes the parsers.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Serhiy Storchaka added 2 commits May 29, 2024 21:29
Serhiy Storchaka
Merge branch 'main' into complex-constructor-docs
28bae65
Serhiy Storchaka
Add an example for truncation.
944dc6c
@serhiy-storchaka serhiy-storchaka mentioned this pull request May 30, 2024
gh-119740: Remove deprecated trunc delegation #119743
Merged
Serhiy Storchaka
Rewrite the description of the complex format and use productionlist.
97a9e00
picnixz
picnixz reviewed May 30, 2024
View reviewed changes
Comment thread Doc/library/functions.rst
Convert a single string or number to a complex number, or create a
complex number from real and imaginary parts.

Examples:

ghost May 30, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Examples:
.. rubric:: Examples

It may look a bit nicer, but I don't know whether you want a smaller heading here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ghost May 30, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking how it is used in Doc/library/asyncio-api-index.rst, I do not think that it is appropriate here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Doc/library/functions.rst
Comment on lines +386 to +401
>>> complex('+1.23')
(1.23+0j)
>>> complex('-4.5j')
-4.5j
>>> complex('-1.23+4.5j')
(-1.23+4.5j)
>>> complex('\t( -1.23+4.5J )\n')
(-1.23+4.5j)
>>> complex('-Infinity+NaNj')
(-inf+nanj)
>>> complex(1.23)
(1.23+0j)
>>> complex(imag=-4.5)
-4.5j
>>> complex(-1.23, 4.5)
(-1.23+4.5j)

ghost May 30, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also expect the most useful to the least useful examples even though it mixes the parsers.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Doc/library/functions.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/functions.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

skirpichev
skirpichev reviewed May 30, 2024
View reviewed changes
Comment thread Doc/library/functions.rst
The string can optionally be surrounded by whitespaces and the round
parenthesis ``'('`` and ``')'``, which are ignored.
The string must not contain whitespace between ``'+'``, ``'-'``, the
``'j'`` or ``'J'`` suffix, and the decimal number.

ghost May 30, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this "j" vs "J" happens in several places. You could just mention (as in the float constructor) that parsing is case-insensitive:

>>> complex('(InFiNITY+1j)')
(inf+1j)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ghost May 30, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But "e" and "E" are mentioned separately in the float grammar. We can say also that all whitespaces except the leading and trailing are not acceptable, but I think it will be clearer to be a little more verbose here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
serhiy-storchaka
serhiy-storchaka commented May 30, 2024
View reviewed changes
Comment thread Doc/library/functions.rst
Convert a single string or number to a complex number, or create a
complex number from real and imaginary parts.

Examples:

ghost May 30, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking how it is used in Doc/library/asyncio-api-index.rst, I do not think that it is appropriate here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Doc/library/functions.rst
The string can optionally be surrounded by whitespaces and the round
parenthesis ``'('`` and ``')'``, which are ignored.
The string must not contain whitespace between ``'+'``, ``'-'``, the
``'j'`` or ``'J'`` suffix, and the decimal number.

ghost May 30, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But "e" and "E" are mentioned separately in the float grammar. We can say also that all whitespaces except the leading and trailing are not acceptable, but I think it will be clearer to be a little more verbose here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Doc/library/functions.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Serhiy Storchaka Bénédikt Tran
Apply suggestions from code review
d77833c
Co-authored-by: Bénédikt Tran <[email protected]>
@serhiy-storchaka
serhiy-storchaka enabled auto-merge (squash) May 30, 2024 19:26
Serhiy Storchaka Bénédikt Tran
Update Doc/library/functions.rst
9664b6a
Co-authored-by: Bénédikt Tran <[email protected]>
serhiy-storchaka
serhiy-storchaka commented May 30, 2024
View reviewed changes
Comment thread Doc/library/functions.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Serhiy Storchaka
Update Doc/library/functions.rst
c23c659
@serhiy-storchaka
serhiy-storchaka merged commit ec1ba26 into python:main May 30, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label May 30, 2024
@miss-islington-app

ghost commented May 30, 2024

Copy link
Copy Markdown

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@miss-islington-app

ghost commented May 30, 2024

Copy link
Copy Markdown

Sorry, @serhiy-storchaka, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker ec1ba264607b2b7b98d2602f5536a1d02981efc6 3.12
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@miss-islington-app miss-islington-app Bot assigned serhiy-storchaka May 30, 2024
@bedevere-app

ghost commented May 30, 2024

Copy link
Copy Markdown

GH-119803 is a backport of this pull request to the 3.13 branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot removed the needs backport to 3.13 only security fixes label May 30, 2024
@serhiy-storchaka
serhiy-storchaka deleted the complex-constructor-docs branch May 30, 2024 20:25
@bedevere-app

ghost commented May 30, 2024

Copy link
Copy Markdown

GH-119805 is a backport of this pull request to the 3.12 branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot removed the needs backport to 3.12 only security fixes label May 30, 2024
@serhiy-storchaka serhiy-storchaka removed their assignment Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@skirpichev skirpichev skirpichev approved these changes
@picnixz picnixz picnixz left review comments
@AlexWaygood AlexWaygood AlexWaygood approved these changes
+1 more reviewer
@mdickinson mdickinson mdickinson approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

docs Documentation in the Doc dir skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@serhiy-storchaka @mdickinson @skirpichev @picnixz @AlexWaygood
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.