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

ENH: Treat 'Inf' as infinity in text parser #4220

Merged
merged 1 commit into from Jul 26, 2013

Conversation

Projects
None yet
4 participants
@jtratner
Copy link
Contributor

commented Jul 12, 2013

Makes 'inf' checks case insensitive using strcasecmp
Fixes #4219.

@jreback

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2013

was using strcasecmp slower? (prob not a big deal either way)

@jtratner

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2013

No. I'm doing that instead. Just found a few other things that needed to be fixed (plus means test cases need to be larger).

Is TestPythonParser supposed to call the parser functions in parsers.pyx?

@jreback

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2013

some of python parser functions are shared

but not sure (your tests are run thru both parsers though so u will know if they work or not)

@jtratner

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2013

Okay, I guess I thought engine='python' would set it to pure python only. Not a big deal to me, just wanted to make sure (thought it could have been a bug or something)

@jtratner

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2013

And here's a perf test between various options: http://nbviewer.ipython.org/5980552

@jreback

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2013

@jtratner you are very thorough!

and you said u were busy this week :)

@jtratner

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2013

haha ... didn't take long to do so I figured I'd crank it out.

This means that any text that converts to 'inf' when lower cased is considered infinity [though only when attempting to convert column to float/double]. Is that what we want to do?

(e.g., "-iNF", "inf", "inF" all would become versions of +/- infinity)

@jreback

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2013

yes I think any of those variations are fine

it might be easier/faster to make a hash table of all of the combinations and just do a lookup
(this is how na values r done) - but only if u r interested in trying it out - prob not necessary

@jtratner

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2013

hmm...I'll think about it. Could probably just reuse the function that creates the hashtable for na_values then. But given that right now it's a 25ish character change in the actual code (import strcasecmp, strcmp-->strcasecmp), this is pretty simple. [just realized I left in the extra (and now unnecessary inf values)]

@jreback

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2013

yep looks good to go

@jtratner

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2013

If you want doc update on this, I can add Monday.

On Thu, Jul 11, 2013 at 10:13 PM, jreback notifications@github.com wrote:

yep looks good to go


Reply to this email directly or view it on GitHubhttps://github.com//pull/4220#issuecomment-20854752
.

@jreback

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2013

hold off on release notes till after 0.12...have to add the new file

@jtratner

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2013

Will do.

@jtratner

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2013

Btw - when are you pulling the trigger on 0.12? :)

@jreback

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2013

just a couple minor issues with 0.12rc1...then should be out

ENH: Use case-insensitive checks for 'inf' in parser.
Instead of just 'inf' or '-inf', can test for 'Inf', '-Inf', 'INF', etc.
Uses strcasecmp under the hood.

(also, small fix to assert_almost_equal to make string comparisons
clearer)
@jtratner

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2013

@jreback This should be ready to go (just waitin' on Travis).

@jreback

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2013

should 'infinity' be there too?

@jtratner

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2013

I'd say no, because that's bordering more on things that could be string-like (not 100% sure: are inf checks are done before dtype decision?). If we did that, would need to switch to hashtable format, which I could do. your call on how you want to do it.

@jreback

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2013

right. so forget that
looks good what u have

can u just do a quick perf test
just run vs 0.12 vbenchs
you can use -r regex to only run some
I some in the io_brench

just to make sure it doesn't slow it down much (I think it'll be fine though)

@jtratner

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2013

@jreback is there a way to collect vbench results into a csv?

I find the my results can vary a lot and being able to combine multiple would help smooth it out.

@jtratner

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2013

Here's perf results - ran it twice (wasn't sure what regex to use):
https://gist.github.com/jtratner/cb114164d3bdce2e4dfa

That said, reading-related items don't seem to be particularly affected (or at least are not negative).

@cpcloud

This comment has been minimized.

Copy link
Member

commented Jul 26, 2013

Make sure you're not doing literally anything else when you run test perf. I sometimes forget that and I get "bad" results. Ratio > 1.15 is usually something sketchy but only if you weren't running anything else.

@cpcloud

This comment has been minimized.

Copy link
Member

commented Jul 26, 2013

Well anything like building docs or compiling stuff. Surfing the web is OK.

@jtratner

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2013

Here are just the "read_*" benches:

*** Processing results...
***
Invoked with :
--ncalls: 3
--repeats: 3


-------------------------------------------------------------------------------
Test name                                    | head[ms] | base[ms] |  ratio   |
-------------------------------------------------------------------------------
read_store_table                             |   3.3030 |   4.4127 |   0.7485 |
read_csv_vb                                  |  29.5163 |  37.5853 |   0.7853 |
read_csv_comment2                            |  29.8669 |  31.6617 |   0.9433 |
read_table_multiple_date                     | 295.3293 | 298.5720 |   0.9891 |
read_store                                   |   2.6910 |   2.6627 |   1.0107 |
read_table_multiple_date_baseline            | 137.8644 | 135.1160 |   1.0203 |
read_store_table_mixed                       |   6.5330 |   6.3693 |   1.0257 |
read_parse_dates_iso8601                     |   1.7890 |   1.7393 |   1.0286 |
read_store_mixed                             |   5.8227 |   5.6427 |   1.0319 |
read_csv_thou_vb                             |  43.4330 |  41.5463 |   1.0454 |
read_store_table_panel                       |  42.0457 |  40.0033 |   1.0511 |
read_store_table_wide                        |  21.0853 |  19.6953 |   1.0706 |
read_csv_standard                            |  18.4487 |  15.5730 |   1.1847 |
-------------------------------------------------------------------------------
Test name                                    | head[ms] | base[ms] |  ratio   |
-------------------------------------------------------------------------------

Ratio < 1.0 means the target commit is faster then the baseline.
Seed used: 1234

Target [3080721] : ENH: Use case-insensitive checks for 'inf' in parser.
Base   [6c8fca9] : Merge pull request #4282 from cpcloud/add-clipboard-test
@jtratner

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2013

I just ran the bench against a bogus commit (literally adding whitespace to ez_setup.py) and I get about the same differences just by random chance, so I'm not convinced that these results are particularly meaningful.

@jtratner

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2013

e.g. here's against a basically blank commit:

-------------------------------------------------------------------------------
Test name                                    | head[ms] | base[ms] |  ratio   |
-------------------------------------------------------------------------------
read_store_table_mixed                       |   6.5234 |   6.8897 |   0.9468 |
read_store_table_panel                       |  40.2050 |  41.2770 |   0.9740 |
read_store                                   |   2.7483 |   2.7854 |   0.9867 |
read_csv_standard                            |  15.7667 |  15.9566 |   0.9881 |
read_csv_comment2                            |  32.8847 |  33.2080 |   0.9903 |
read_store_table                             |   3.2610 |   3.2636 |   0.9992 |
read_store_mixed                             |   5.7760 |   5.6917 |   1.0148 |
read_csv_thou_vb                             |  43.5576 |  42.8537 |   1.0164 |
read_parse_dates_iso8601                     |   1.7897 |   1.7587 |   1.0177 |
read_store_table_wide                        |  20.0947 |  19.7203 |   1.0190 |
read_table_multiple_date_baseline            | 141.1477 | 137.1057 |   1.0295 |
read_table_multiple_date                     | 317.5010 | 296.3533 |   1.0714 |
read_csv_vb                                  |  34.2953 |  29.2070 |   1.1742 |
-------------------------------------------------------------------------------
Test name                                    | head[ms] | base[ms] |  ratio   |
-------------------------------------------------------------------------------
@jreback

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2013

perf testing is actually quite tricky as many things can affect it from one run to another - just looking to make sure that the change which we think is a non event really is

so looks good then

jreback added a commit that referenced this pull request Jul 26, 2013

Merge pull request #4220 from jtratner/expand-inf-comparisons
ENH: Treat 'Inf' as infinity in text parser

@jreback jreback merged commit 562b4bd into pandas-dev:master Jul 26, 2013

@jreback

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2013

thank you sir!

@dieterv77

This comment has been minimized.

Copy link
Contributor

commented on pandas/parser.pyx in 3080721 Jul 26, 2013

strcasecmp does not exist on the Windows compilers, so this does not build. I will take a look this weekend about making this portable, Windows compilers have a function called _stricmp instead.

@jtratner jtratner deleted the jtratner:expand-inf-comparisons branch Sep 21, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.