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

Parse floats using Parsers.jl (fixes #284) #285

Merged
merged 3 commits into from
Jun 1, 2019
Merged

Conversation

kmsquire
Copy link
Contributor

On my machine, this gives a good speedup on the (minimal) benchmarks that we have (and we now are faster than Python!):

Before:

$ julia1.1 bench.jl parse -f
 [BenchF] canada 0.053259659 seconds
 [BenchF] citm_catalog 0.010465748 seconds
 [BenchF] citylots 3.622005479 seconds
 [BenchF] twitter 0.004414869 seconds
 [BenchF] Total (G.M.) 0.0546397740591351 seconds

After:

$ julia1.1 bench/bench.jl parse -f
 [BenchF] canada 0.034515661 seconds
 [BenchF] citm_catalog 0.009896403 seconds
 [BenchF] citylots 3.371572698 seconds
 [BenchF] twitter 0.003937035 seconds
 [BenchF] Total (G.M.) 0.04614491419775058 seconds

Python:

$ python3 bench.py
canada 0.040348 seconds
citm_catalog 0.011868 seconds
citylots 3.509695 seconds
twitter 0.005907 seconds
Total (G.M): 0.056133

Cc: @quinnj @TotalVerb

@kmsquire kmsquire requested review from quinnj and TotalVerb May 30, 2019 00:59
@kmsquire
Copy link
Contributor Author

I'll pull the transition to Project.toml out into its own pull request (except for the extra dependencies pulled in by Parsers.jl).

@kmsquire
Copy link
Contributor Author

One Appveyor build failed because of a DNS lookup (unrelated). I hesitate to close and open just to run everything again, especially if I should make any changes, and I don't have permissions to restart just that build.

Copy link
Member

@quinnj quinnj left a comment

Choose a reason for hiding this comment

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

This looks good to me! Yeah, I understand the comment w/ wanting to use tryparse and I've been meaning to update things to allow consuming the full input.

@TotalVerb
Copy link
Collaborator

I'm not sure I have permission to restart appveyor either. I'm fine with the spurious failure since it's on 0.7 anyway.

@kmsquire kmsquire merged commit 1231b52 into master Jun 1, 2019
@kmsquire kmsquire deleted the kms/parsers-jl branch June 1, 2019 23:42
@tlnagy
Copy link

tlnagy commented Sep 10, 2019

Unfortunately, it looks like this PR was included in a JSON.jl release that still has support for 0.7 listed in its compat section in the Project.toml so it broke our tests over on Gadfly (actually in Compose.jl: https://travis-ci.org/GiovineItalia/Gadfly.jl/jobs/583375273) on Julia 0.7.

Here: https://github.com/JuliaRegistries/General/pull/2060/files#diff-68074356855d5796422b09f38b90317fR41

I'll open an issue over there to get this fixed retroactively.

Looks like I was mistaken and the failure on the Compose build that pointed at the xparse line introduced by this PR was a false flag. Not sure what's causing the failure, but I'm trying to work that out now. Sorry for the spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants