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

add parse(Complex{T}, s) #24713

Merged
merged 13 commits into from
Dec 6, 2017
Merged

add parse(Complex{T}, s) #24713

merged 13 commits into from
Dec 6, 2017

Conversation

stevengj
Copy link
Member

@stevengj stevengj commented Nov 22, 2017

Closes #22250. parse(Complex{T}, s::String) now works, supporting a ± bIM for IM in (i,j,im), with arbitrary whitespace.

Closes #21935. readdlm(io, ',', Complex{T}) now works.

I also needed a fix for parse(BigFloat, s), because it was failing if s had trailing whitespace (which is allowed by other number parsers in Julia). Added a test, and tightened up the BigFloat parsing tests (which were using when they could/should have been using ==).

@stevengj stevengj requested a review from ararslan November 22, 2017 22:06
@ararslan ararslan added the complex Complex numbers label Nov 22, 2017
@@ -229,3 +229,21 @@ end
@test tryparse(Float32, "1.23") === Nullable(1.23f0)
@test tryparse(Float16, "1.23") === Nullable(Float16(1.23))

# parsing complex numbers (#22250)
@testset "complex parsing" begin
Copy link
Member

Choose a reason for hiding this comment

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

It might also be good to add some tests for parsing bogus stuff, like 1+2ij or 1im-3im.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@ararslan
Copy link
Member

I'm not entirely sold on being able to parse things that aren't actual Julia values, e.g. 1+2j, since we don't seem to do that for other types as far as I can tell. I can see how it would be useful for interoperability with languages that don't use im though.

@stevengj
Copy link
Member Author

stevengj commented Nov 23, 2017

@ararslan, my sense is that parse(T, s) is almost never exactly equivalent to parsing Julia code.

Sometimes this means parsing a subset of valid Julia expressions, e.g. parse(Int, "1_000") throws an error. But sometimes it means parsing something completely different, e.g. parse(Date, "2018-4-1") has nothing to do with how Julia parses the expression 2018-4-1.

In the case of parsing complex numbers, I would say that if the user writes parse(Complex{Int}, "3+4j"), then the intended result is unambiguous and very useful for interop. Also, supporting this small superset of Julia complex syntax costs almost nothing.

@ararslan
Copy link
Member

Okay, now I'm sold.

@stevengj
Copy link
Member Author

I updated the implementation to extend tryparse_internal rather than tryparse. This has the advantage that it can print more informative error messages in parse, and also that it can avoid allocating SubString objects for most of the built-in types. This required adding some additional tryparse_internal methods for other real types too.

As a side effect, I was able to shorten some of the readdlm code and fix #21935.

@stevengj
Copy link
Member Author

stevengj commented Nov 23, 2017

AppVeyor is failing with could not find function ssyconvf_rook_64_ in library libopenblas64_, which seems unrelated. Travis 32-bit Linux is passing, but Travis 64-bit Linux is failing with

  Expression: libgit2
  Error deserializing a remote exception from worker 5
  Remote(original) exception of type Test.TestSetException

which again seems unrelated? Travis OSX is failing with the unrelated error error compiling __init__: error compiling check: could not load library "libopenblas".

The unreliability of CI these days really makes PRs difficult; it feels like it's been months since I've seen a completely green CI. 😢

@ararslan
Copy link
Member

Yeah the CI situation nowadays is pretty dire. I believe Curtis Vogt has a fix in the works for the libgit2 stuff, which should fix the issues on Linux, Windows, and FreeBSD. I've been seeing the OpenBLAS issue on Travis macOS pretty consistently lately; I wonder if the version of GCC in Homebrew changed and now versions aren't matching properly or something.

@stevengj
Copy link
Member Author

stevengj commented Dec 5, 2017

Pushed a slight change to NEWS; let's see if CI is better now.

@ararslan
Copy link
Member

ararslan commented Dec 5, 2017

64-bit Windows and macOS failures are unrelated.

@stevengj
Copy link
Member Author

stevengj commented Dec 6, 2017

Okay to merge, then?

@ararslan ararslan merged commit 0c5b971 into JuliaLang:master Dec 6, 2017
@stevengj stevengj deleted the parsecomplex branch December 7, 2017 01:46
evetion pushed a commit to evetion/julia that referenced this pull request Dec 12, 2017
JeffBezanson pushed a commit that referenced this pull request Oct 20, 2023
This fixes a string-indexing bug introduced in #24713 (Julia 0.7).
Sometimes, this would cause `parse(Complex{T}, string)` to throw a
`StringIndexError` rather than an `ArgumentError`, e.g. for
`parse(ComplexF64, "3 β+ 4im")` or `parse(ComplexF64, "3 + 4αm")`. (As
far as I can tell, it can never cause parsing to fail for valid
strings.)

The source of the error is that if `i` is the index of an ASCII
character in a string `s`, then `s[i+1]` is valid (even if the next
character is non-ASCII) but `s[i-1]` is invalid if the previous
character is non-ASCII.
KristofferC pushed a commit that referenced this pull request Oct 23, 2023
This fixes a string-indexing bug introduced in #24713 (Julia 0.7).
Sometimes, this would cause `parse(Complex{T}, string)` to throw a
`StringIndexError` rather than an `ArgumentError`, e.g. for
`parse(ComplexF64, "3 β+ 4im")` or `parse(ComplexF64, "3 + 4αm")`. (As
far as I can tell, it can never cause parsing to fail for valid
strings.)

The source of the error is that if `i` is the index of an ASCII
character in a string `s`, then `s[i+1]` is valid (even if the next
character is non-ASCII) but `s[i-1]` is invalid if the previous
character is non-ASCII.

(cherry picked from commit f71228d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
complex Complex numbers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add parse(Complex{T}, string) readdlm does not support Complex
2 participants