Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

strconv: add ParseComplex #36771

Closed
pjebs opened this issue Jan 26, 2020 · 36 comments
Closed

strconv: add ParseComplex #36771

pjebs opened this issue Jan 26, 2020 · 36 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted
Milestone

Comments

@pjebs
Copy link
Contributor

pjebs commented Jan 26, 2020

strconv.ParseComplex(s string, bitSize int) (complex128, error)

@gopherbot gopherbot added this to the Proposal milestone Jan 26, 2020
@pjebs
Copy link
Contributor Author

pjebs commented Jan 26, 2020

I don't think it should import regexp or strings to keep the package small. It is heavily recommended (instead of fmt) by gopherjs for exactly this reason.

@josharian
Copy link
Contributor

Exactly what format(s) do you think it should accept?

@ianlancetaylor
Copy link
Contributor

Note that the language doesn't define any format for a complex expression. Instead it defines an imaginary literal (an integer or floating point constant followed by i). Any complex constant is simply a constant expression that includes at least one imaginary literal. So I think @josharian is asking a good question here. We don't want the strconv package to have to parse an arbitrary expression.

That said, we do have an implicit definition for parsing a complex constant in fmt.Scan, implemented in the method fmt.(*ss).complexTokens (https://golang.org/src/fmt/scan.go#L740). This syntax doesn't seem to have any user-visible documentation. We could decide to move that method over to strconv.ParseComplex.

@pjebs
Copy link
Contributor Author

pjebs commented Jan 27, 2020

Let me know with what I've done so far after addressing your concerns.

PR: #36815

@pjebs
Copy link
Contributor Author

pjebs commented Jan 27, 2020

It will accept:

- (±34.3±234.2i)
- ±34.3±234.2i
- exponential form
- hexadecimal form
- NaN
- ±Inf±Infi
- only real
- only imag

@gopherbot
Copy link

Change https://golang.org/cl/216617 mentions this issue: strconv: Add ParseCompex function

@odeke-em odeke-em changed the title Proposal: strconv: ParseComplex proposal: strconv: add ParseComplex Jan 29, 2020
@pjebs
Copy link
Contributor Author

pjebs commented Mar 22, 2020

Any news on this?

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Mar 22, 2020
@rsc
Copy link
Contributor

rsc commented Mar 25, 2020

fmt.Print and fmt.Scan do have an implicit format for complex numbers:
FLOAT+/-FLOATi or (FLOAT+/-FLOATi)
They format and accept both decimal and hex floats, and NaN and Inf for either or both FLOATs.

For floats we have:

func ParseFloat(s string, bitSize int) (float64, error)
func FormatFloat(f float64, fmt byte, prec, bitSize int) string

It seems like we'd need

func ParseComplex(s string, bitSize int) (complex128, error)
func FormatComplex(c complex128, fmt byte, prec, bitSize int) string

This seems like it is a hole we forgot to fill.

/cc @robpike @griesemer

@robpike
Copy link
Contributor

robpike commented Mar 25, 2020

Seems like a reasonable thing to add, in parallel with float64 support as @rsc says. It would be good to see if the fmt package adapts well to using it.

@pjebs
Copy link
Contributor Author

pjebs commented Mar 25, 2020

What should FormatComplex return?

  • With brackets or without brackets?
  • hex or always decimal?

@rsc rsc moved this from Incoming to Active in Proposals (old) Mar 26, 2020
@rsc
Copy link
Contributor

rsc commented Mar 26, 2020

FormatComplex should probably put in the parens, same as fmt.
The question of hex or decimal is controlled by the fmt byte,
like in Printf: 'x' gets hex.

For example, I believe these tests from fmt show what fmt='x' prec=3 should do:

fmt_test.go:532:	{"%.3x", 0i, "(0x0.000p+00+0x0.000p+00i)"},
fmt_test.go:538:	{"%.3x", 1 + 2i, "(0x1.000p+00+0x1.000p+01i)"},
fmt_test.go:542:	{"%.3x", -1 - 2i, "(-0x1.000p+00-0x1.000p+01i)"},

@pjebs
Copy link
Contributor Author

pjebs commented Mar 27, 2020

cool. implemented FormatComplex. Will do unit tests later.

@rsc
Copy link
Contributor

rsc commented Apr 1, 2020

Based on the discussion, this seems like a likely accept.

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Apr 1, 2020
@rsc
Copy link
Contributor

rsc commented Apr 8, 2020

No change in consensus, so accepted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Apr 8, 2020
@rsc rsc modified the milestones: Proposal, Backlog Apr 8, 2020
@rsc rsc changed the title proposal: strconv: add ParseComplex strconv: add ParseComplex Apr 8, 2020
@pjebs
Copy link
Contributor Author

pjebs commented Apr 10, 2020

So what happens now? Well it be in go 1.15?

@ianlancetaylor
Copy link
Contributor

Accepting the proposal means that it is OK to make the change. In this case you've already sent a CL, so now the appropriate maintainers need to review it. It can be in 1.15 if it gets reviewed and accepted.

@ianlancetaylor ianlancetaylor added the NeedsFix The path to resolution is known, but the work has not been done. label Apr 10, 2020
@odeke-em
Copy link
Member

Here is an initial prototype using the logic already in strconv.ParseFloat https://play.golang.org/p/sK3o9iBNJ55. @pjebs I shall take a look at your CL.

@pjebs
Copy link
Contributor Author

pjebs commented Apr 12, 2020

Is it possible to keep strings package out because it's heavily used in environments like gopher.js @odeke-em

@odeke-em
Copy link
Member

For sure, @pjebs, thanks for letting me know :) Let's take it to your CL, where I've posted feedback.

@pjebs
Copy link
Contributor Author

pjebs commented Apr 12, 2020

@odeke-em Thank you for the feedback. I'll make various adjustments in accordance with your suggestions. I was told we were only accepting (N±Ni) format as per #36771 (comment)

@odeke-em
Copy link
Member

odeke-em commented Apr 12, 2020 via email

@pjebs
Copy link
Contributor Author

pjebs commented Apr 12, 2020

I got confused because the @ianlancetaylor pointed me to the "implicit definition" that Go currently accepts and it states:

// complexTokens returns the real and imaginary parts of the complex number starting here.
// The number might be parenthesized and has the format (N+Ni) where N is a floating-point
// number and there are no spaces within.

My interpretation was that there is already code that only parses/accepts (N+Ni) and @ianlancetaylor wants to replace that existing code (in fmt) with the new strconv.ParseComplex, in which case it will create a break to the Go1 backward compatibility policy.

@ianlancetaylor
Copy link
Contributor

The format we accepted for this proposal is exactly the one in #36771 (comment) , which is simpler and more constraining than what @odeke-em suggests above.

@odeke-em
Copy link
Member

odeke-em commented Apr 12, 2020 via email

@pjebs
Copy link
Contributor Author

pjebs commented Apr 12, 2020

In that case, @odeke-em can you remove the comments you made that are related to the expanded definition. I believe there were only cosmetic adjustments left.

@odeke-em
Copy link
Member

Acknowledged, thank you @pjebs and @ianlancetaylor for the clarification. I shall update my comments on the CL.

@pjebs
Copy link
Contributor Author

pjebs commented Apr 20, 2020

@odeke-em Finished amending PR based on your comments.

The only thing I didn't change was your suggestion that I change IEEE754 to IEEE 754 because the ParseComplex doc comments are directly from ParseFloat function and that's how it was in there.

Thanks for the review.
Also noticed to my surprise that you star'd my https://github.com/rocketlaunchr/dbq package.

@odeke-em
Copy link
Member

@odeke-em Finished amending PR based on your comments.

Awesome, I shall take a look and I'll also kindly rope in @griesemer for the final look.

Also noticed to my surprise that you star'd my https://github.com/rocketlaunchr/dbq package.

Aye aye. Nice work and ergonimcs, I hope to use it sometime in the future!

@pjebs
Copy link
Contributor Author

pjebs commented Apr 20, 2020

The code already (implicitly) rejects malformed parenthesis

@pjebs
Copy link
Contributor Author

pjebs commented Apr 21, 2020

@odeke-em

Issues:

PS8, Line 10
This suggestion wasn't added.

Am I meant to change the commit message for my initial commit? Is that how I do it?

We need to ensure that malformed parenthesis throw out an error e.g.:

The ParseFloat function already takes care of rejecting malformed parenthesis. There is no need to cater for it explicitly. See test cases.

This code isn't intuitive to me on what it does.
Would be nice to explain the logic of how we figure out what's real what's imaginary.

I have added more detailed comments to explain the logic. I feel that the algorithmic approach is simpler once the logic is understood.

@odeke-em
Copy link
Member

odeke-em commented Apr 21, 2020 via email

@pjebs
Copy link
Contributor Author

pjebs commented Apr 26, 2020

@odeke-em I responded on the official site.

@pjebs
Copy link
Contributor Author

pjebs commented Apr 27, 2020

@ianlancetaylor @griesemer Will this make it to Go1.15?

@griesemer
Copy link
Contributor

I've started looking at the code.

@gopherbot
Copy link

Change https://golang.org/cl/230737 mentions this issue: strconv: implement parseFloat returning no. of bytes consumed

gopherbot pushed a commit that referenced this issue Apr 30, 2020
parseFloatPrefix will make it easier to implement ParseComplex.

Verified that there's no relevant performance impact:
Benchmarks run on a "quiet" MacBook Pro, 3.3GHz Dual-Core Intel Core i7,
with 16GB 2133MHz LPDDR3 RAM running macOS 10.15.4.

name                  old time/op  new time/op  delta
Atof64Decimal-4       38.2ns ± 4%  38.4ns ± 3%    ~     (p=0.802 n=5+5)
Atof64Float-4         41.1ns ± 3%  43.0ns ± 1%  +4.77%  (p=0.008 n=5+5)
Atof64FloatExp-4      71.9ns ± 3%  70.1ns ± 1%    ~     (p=0.063 n=5+5)
Atof64Big-4            124ns ± 5%   119ns ± 0%    ~     (p=0.143 n=5+4)
Atof64RandomBits-4    57.2ns ± 1%  55.7ns ± 2%  -2.66%  (p=0.016 n=4+5)
Atof64RandomFloats-4  56.8ns ± 1%  56.9ns ± 4%    ~     (p=0.556 n=4+5)
Atof32Decimal-4       35.4ns ± 5%  35.9ns ± 0%    ~     (p=0.127 n=5+5)
Atof32Float-4         39.6ns ± 7%  40.3ns ± 1%    ~     (p=0.135 n=5+5)
Atof32FloatExp-4      73.7ns ± 7%  71.9ns ± 0%    ~     (p=0.175 n=5+4)
Atof32Random-4         103ns ± 6%    98ns ± 2%  -5.03%  (p=0.008 n=5+5)

Updates #36771.

Change-Id: I8ff66b582ae8b468d89c9ffc35c569c735cf0341
Reviewed-on: https://go-review.googlesource.com/c/go/+/230737
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/231237 mentions this issue: strconv: fix for parseFloatPrefix

gopherbot pushed a commit that referenced this issue Apr 30, 2020
parseFloatPrefix accepts a string if it has a valid floating-point
number as prefix. Make sure that "infi", "infin", ... etc. are
accepted as valid numbers "inf" with suffix "i", "in", etc. This
is important for parsing complex numbers such as "0+infi".

This change does not affect the correctness of ParseFloat because
ParseFloat rejects strings that contain a suffix after a valid
floating-point number.

Updates #36771.

Change-Id: Ie1693a8ca2f8edf07b57688e0b35751b7100d39d
Reviewed-on: https://go-review.googlesource.com/c/go/+/231237
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
xujianhai666 pushed a commit to xujianhai666/go-1 that referenced this issue May 21, 2020
parseFloatPrefix will make it easier to implement ParseComplex.

Verified that there's no relevant performance impact:
Benchmarks run on a "quiet" MacBook Pro, 3.3GHz Dual-Core Intel Core i7,
with 16GB 2133MHz LPDDR3 RAM running macOS 10.15.4.

name                  old time/op  new time/op  delta
Atof64Decimal-4       38.2ns ± 4%  38.4ns ± 3%    ~     (p=0.802 n=5+5)
Atof64Float-4         41.1ns ± 3%  43.0ns ± 1%  +4.77%  (p=0.008 n=5+5)
Atof64FloatExp-4      71.9ns ± 3%  70.1ns ± 1%    ~     (p=0.063 n=5+5)
Atof64Big-4            124ns ± 5%   119ns ± 0%    ~     (p=0.143 n=5+4)
Atof64RandomBits-4    57.2ns ± 1%  55.7ns ± 2%  -2.66%  (p=0.016 n=4+5)
Atof64RandomFloats-4  56.8ns ± 1%  56.9ns ± 4%    ~     (p=0.556 n=4+5)
Atof32Decimal-4       35.4ns ± 5%  35.9ns ± 0%    ~     (p=0.127 n=5+5)
Atof32Float-4         39.6ns ± 7%  40.3ns ± 1%    ~     (p=0.135 n=5+5)
Atof32FloatExp-4      73.7ns ± 7%  71.9ns ± 0%    ~     (p=0.175 n=5+4)
Atof32Random-4         103ns ± 6%    98ns ± 2%  -5.03%  (p=0.008 n=5+5)

Updates golang#36771.

Change-Id: I8ff66b582ae8b468d89c9ffc35c569c735cf0341
Reviewed-on: https://go-review.googlesource.com/c/go/+/230737
Reviewed-by: Ian Lance Taylor <iant@golang.org>
xujianhai666 pushed a commit to xujianhai666/go-1 that referenced this issue May 21, 2020
parseFloatPrefix accepts a string if it has a valid floating-point
number as prefix. Make sure that "infi", "infin", ... etc. are
accepted as valid numbers "inf" with suffix "i", "in", etc. This
is important for parsing complex numbers such as "0+infi".

This change does not affect the correctness of ParseFloat because
ParseFloat rejects strings that contain a suffix after a valid
floating-point number.

Updates golang#36771.

Change-Id: Ie1693a8ca2f8edf07b57688e0b35751b7100d39d
Reviewed-on: https://go-review.googlesource.com/c/go/+/231237
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@golang golang locked and limited conversation to collaborators May 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

8 participants