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

DEPR: Deprecate as_recarray in read_csv #13373

Closed

Conversation

gfyoung
Copy link
Member

@gfyoung gfyoung commented Jun 5, 2016

  1. Documented and deprecate as_recarray
  2. Added as_recarray functionality to Python engine
  3. Fixed bug in C engine in which usecols was not being respected in combination with as_recarray

@gfyoung gfyoung force-pushed the as-recarray-python-engine branch from c76f2e6 to e71aa1e Compare June 5, 2016 22:03
@jreback
Copy link
Contributor

jreback commented Jun 5, 2016

check this on Windows

I had disabled a number of these tests because of weird crashing

@gfyoung
Copy link
Member Author

gfyoung commented Jun 5, 2016

I don't have Windows 64 though, which is what I think was crashing based on the SkipTest message. Don't we have Appveyor to check this for us (I notice that it isn't a requirement for merging PR's however)?

@jreback
Copy link
Contributor

jreback commented Jun 5, 2016

ok, sign up for appveyor, we auto run on this, its a free account, and I think that you can even remote login to debug.

In any event I remove the skip for the tests I marked on win64.

Also remove the test marked for 2.6 (its a slow one).

It passed for me on 3.5 on win-64, but let's see what appveyor says.

@jreback jreback added Docs IO CSV read_csv, to_csv labels Jun 5, 2016
@gfyoung
Copy link
Member Author

gfyoung commented Jun 5, 2016

Okay, so if I understand this correctly, remove the SkipTest on test_compact_ints_as_recarray as well as the one on test_file in common.py?

@jreback
Copy link
Contributor

jreback commented Jun 5, 2016

yep

@codecov-io
Copy link

codecov-io commented Jun 5, 2016

Current coverage is 84.24%

Merging #13373 into master will increase coverage by <.01%

@@             master     #13373   diff @@
==========================================
  Files           138        138          
  Lines         50739      50749    +10   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          42740      42750    +10   
  Misses         7999       7999          
  Partials          0          0          

Powered by Codecov. Last updated by 27448d9...e71aa1e

@gfyoung gfyoung force-pushed the as-recarray-python-engine branch from e71aa1e to e7174d4 Compare June 6, 2016 04:25
@gfyoung
Copy link
Member Author

gfyoung commented Jun 6, 2016

@jreback : Removed the SkipTest as requested, and Travis is happy. Ready to merge if there is nothing else.

@jreback
Copy link
Contributor

jreback commented Jun 6, 2016

what about appveyor ?

@jreback
Copy link
Contributor

jreback commented Jun 6, 2016

@gfyoung can you make the update that @jorisvandenbossche requested here: #13360 (IOW, just edit buffer_lines)

@jreback jreback added this to the 0.18.2 milestone Jun 6, 2016
@jorisvandenbossche
Copy link
Member

Broader question: do we actually want to support this feature? (if we want to prune in read_csv keywords, this is maybe a good candidate). Not sure how much used this is.

Has this a significant performance/memory benefit over just using read_csv and to_records afterwards? (I didn't look at the implementation, but I can imagine this could be a reason to keep the keyword)

@jreback
Copy link
Contributor

jreback commented Jun 6, 2016

@jorisvandenbossche good point. yes I think we should deprecate this as well.

@gfyoung xref to #10631 (check box 1) for fixing those tests. The test suite works for me on windows 64 (2.7 & 3.5)

@gfyoung
Copy link
Member Author

gfyoung commented Jun 6, 2016

@jreback : Appveyor passed for me as well, so I think we can consider that Windows issue resolved?

@jorisvandenbossche : That's a good point. I'll add the deprecation too. That also would help with decoupling pandas from numpy as well.

@gfyoung gfyoung changed the title ENH: Support as_recarray better in read_csv DEPR: Deprecate as_recarray better in read_csv Jun 6, 2016
@gfyoung gfyoung force-pushed the as-recarray-python-engine branch from e7174d4 to 6885bca Compare June 6, 2016 14:18
DEPRECATED: this argument will be removed in a future version. Please call
``pd.read_csv(...).to_records()`` instead.

Return a NumPy recarray instead of a DataFrame after parsing the data. If
Copy link
Contributor

Choose a reason for hiding this comment

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

do we test these guarantees?

Copy link
Member Author

Choose a reason for hiding this comment

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

Of course. The test I added here speaks for itself.

@jreback jreback mentioned this pull request Jun 6, 2016
3 tasks
@gfyoung gfyoung changed the title DEPR: Deprecate as_recarray better in read_csv DEPR: Deprecate as_recarray in read_csv Jun 6, 2016
1) Documented and deprecate as_recarray
2) Added as_recarray functionality to Python engine
3) Fixed bug in C engine in which usecols was not
   being respected in combination with as_recarray
@gfyoung gfyoung force-pushed the as-recarray-python-engine branch from 6885bca to abaeaef Compare June 6, 2016 16:49
@gfyoung
Copy link
Member Author

gfyoung commented Jun 6, 2016

@jreback @jorisvandenbossche : deprecated as_recarray, made the documentation fix from my previous PR, and Travis is happy. Ready to merge if there are no other concerns.

@jreback jreback closed this in b1bfd2f Jun 6, 2016
@gfyoung gfyoung deleted the as-recarray-python-engine branch June 6, 2016 23:16
@jreback
Copy link
Contributor

jreback commented Jun 6, 2016

ty

gfyoung added a commit to forking-repos/pandas that referenced this pull request Dec 15, 2017
jreback pushed a commit that referenced this pull request Dec 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Docs IO CSV read_csv, to_csv
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants