Skip to content

FIX: StataReader #3935

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

Merged
merged 1 commit into from
Jun 21, 2013
Merged

FIX: StataReader #3935

merged 1 commit into from
Jun 21, 2013

Conversation

PKEuS
Copy link
Contributor

@PKEuS PKEuS commented Jun 17, 2013

Fix for a bug in StataReader resulting in errors when reading Stata file with string columns

@jreback
Copy link
Contributor

jreback commented Jun 17, 2013

can you add a test case for this?

e.g. write a df with multi-dtypes (including object), and read back in to compare?

@PKEuS
Copy link
Contributor Author

PKEuS commented Jun 17, 2013

Thank you for the suggestion - I was already thinking about how I can create a proper unit test, since I couldn't just use the real-world dataset where the issue happened first.

I have adapted the commit, now also containing a fix for StataWriter that was necessary to make the unit tests work.

@jreback
Copy link
Contributor

jreback commented Jun 17, 2013

as an aside (and this could easily be put in 0.12)

I think it makes sense to support index_col in read_stata, and as_index=False in to_stata, though I am not sure if you can store any meta-data in the dta file? (e.g. to indicate this column)?

@jreback
Copy link
Contributor

jreback commented Jun 17, 2013

also....I am not sure how much support stata has for dates, be sure to convert to/from datetime64[ns] let me know if you have any issues with this

keep in mind that datetime64[ns] CAN have NaT in there (equivalent of nan). You are of course free to raise if you need to if you encounter these (if stats cannot support), again I don't know the limitations

@PKEuS
Copy link
Contributor Author

PKEuS commented Jun 18, 2013

Updated. Now also testing datetime.

@jreback
Copy link
Contributor

jreback commented Jun 20, 2013

this ready to go? (the failure is unrelated to you)

@PKEuS
Copy link
Contributor Author

PKEuS commented Jun 20, 2013

I think this is ready.

@jreback
Copy link
Contributor

jreback commented Jun 20, 2013

great will merge in a bit

jreback added a commit that referenced this pull request Jun 21, 2013
@jreback jreback merged commit 4618e70 into pandas-dev:master Jun 21, 2013
@jreback
Copy link
Contributor

jreback commented Jun 21, 2013

thanks again!

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.

2 participants