Skip to content

remove read_excel keyword NotImplemented error, update documentation #11544 #12051

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

remove read_excel keyword NotImplemented error, update documentation #11544 #12051

wants to merge 1 commit into from

Conversation

grahamjeffries
Copy link
Contributor

@grahamjeffries grahamjeffries commented Jan 15, 2016 •
edited by jorisvandenbossche
Loading

Followed up in #14326

Towards fixing bug #11544, with respect to comments on merged PR #11870.

I've removed the NotImplemented error for parse_dates and date_parser keywords in read_excel, seeing that they are implemented (see discussion in #11870). I've added an example of the intended functionality in the documentation

@grahamjeffries grahamjeffries mentioned this pull request Jan 15, 2016
Merged
@jorisvandenbossche jorisvandenbossche added the IO Excel read_excel, to_excel label Jan 15, 2016
@jorisvandenbossche jorisvandenbossche added this to the 0.18.0 milestone Jan 15, 2016
jorisvandenbossche
jorisvandenbossche reviewed Jan 15, 2016
View reviewed changes
doc/source/io.rst
+++++++++++++

The `parse_dates` keyword for `read_excel` is used to specify whether to parse strings
to a datetime.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe note specifically that you do no need to use this when the data are formatted as dates in the excel file.

@jorisvandenbossche
Copy link
Member

@grahamjeffries Thanks for working on this!

I am only not sure why you changed many of the tests by adding parse_dates=True. Why is that needed?
Besides that, I think it would be good to add a test that actually tests the parse_dates keyword behaviour.

@grahamjeffries
Copy link
Contributor Author

Okay, I'll change the warning so that it is raised only when parse_dates is True, add a test, and and update the documentation example.

Re: changes to the tests, in PR #11870 I removed those arguments in the test to avoid raising the NotImplemented error which is raised by the read_excel modifications in that same PR. Now, I've removed those modifications so I thought I'd add the arguments back. It turns out that it makes no difference to the testing outcomes, so I'm happy to go either way. Please advise which route is preferable.

@jorisvandenbossche
Copy link
Member

For the tests, if it does not make a difference, you don't need to add this back. Rather try to add a test where it does make a difference, so we test the functionality explicitly.
Eg for the test1.xlsx, the first column is formatted as datetime in excel, not as a string, so indeed in those cases parse_dates=True is not needed.

@jreback
Copy link
Contributor

jreback commented Jan 24, 2016

@grahamjeffries can you update

@jreback
Copy link
Contributor

jreback commented Feb 1, 2016

can you update according to @jorisvandenbossche comments (and rebase)

@jreback jreback modified the milestones: 0.18.1, 0.18.0 Feb 9, 2016
@jreback jreback mentioned this pull request Mar 2, 2016
Closed
@jorisvandenbossche jorisvandenbossche modified the milestones: 0.18.0, 0.18.1 Mar 6, 2016
@jorisvandenbossche jorisvandenbossche mentioned this pull request Mar 6, 2016
Closed
@jorisvandenbossche
Copy link
Member

@grahamjeffries Do you have time to update this?

@jreback
Copy link
Contributor

jreback commented Mar 7, 2016

this would be nice for 0.18.0, but not going to hold us up

@jreback jreback modified the milestones: 0.18.1, 0.18.0 Mar 7, 2016
@jreback jreback mentioned this pull request Mar 21, 2016
Closed
jreback
jreback reviewed Apr 25, 2016
View reviewed changes
doc/source/whatsnew/v0.18.0.txt
@@ -481,7 +481,7 @@ Bug Fixes
- Bug in ``df.replace`` while replacing value in mixed dtype ``Dataframe`` (:issue:`11698`)
- Bug in ``Index`` prevents copying name of passed ``Index``, when a new name is not provided (:issue:`11193`)
- Bug in ``read_excel`` failing to read any non-empty sheets when empty sheets exist and ``sheetname=None`` (:issue:`11711`)
- Bug in ``read_excel`` failing to raise ``NotImplemented`` error when keywords ``parse_dates`` and ``date_parser`` are provided (:issue:`11544`)
- Bug in ``read_excel`` failing to raise warning when keyword ``parse_dates`` and is provided without keyword ``index_col`` (:issue:`11544`)
Copy link
Contributor

Choose a reason for hiding this comment

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

move to 0.18.1

@jreback jreback removed this from the 0.18.1 milestone Apr 25, 2016
@jreback
Copy link
Contributor

jreback commented May 20, 2016

@grahamjeffries can you rebase / update

@jorisvandenbossche jorisvandenbossche added this to the 0.19.0 milestone Jul 25, 2016
@jorisvandenbossche
Copy link
Member

@grahamjeffries Do you have some time to update/rebase this?

@jreback
Copy link
Contributor

jreback commented Aug 18, 2016

status of this?

needs a rebase

@jorisvandenbossche
Copy link
Member

Either this should be merged, or I should fix the docs that use this keyword (now error during doc build), so I would keep it at the 0.19.0 milestone for now to remember that

@jreback
Copy link
Contributor

jreback commented Sep 9, 2016

can you rebase / update?

@jreback
Copy link
Contributor

jreback commented Sep 20, 2016

@grahamjeffries can you rebase / update this?

@jreback jreback modified the milestones: 0.19.0, 0.19.1 Sep 28, 2016
@jorisvandenbossche jorisvandenbossche mentioned this pull request Sep 30, 2016
Closed
@jorisvandenbossche
Copy link
Member

I rebased and updated this in #14326

@jorisvandenbossche jorisvandenbossche modified the milestones: No action, 0.19.1 Sep 30, 2016
jreback pushed a commit to jreback/pandas that referenced this pull request Mar 27, 2017
@jorisvandenbossche @jreback
Remove NotImplementedError for parse_dates keyword in read_excel
10d8c40 
Rebase and update of PR pandas-dev#12051

Author: Joris Van den Bossche <[email protected]>
Author: Graham R. Jeffries <[email protected]>

This patch had conflicts when merged, resolved by
Committer: Jeff Reback <[email protected]>

Closes pandas-dev#14326 from jorisvandenbossche/pr/12051 and squashes the following commits:

0b65a7a [Joris Van den Bossche] update wording
656ec44 [Joris Van den Bossche] Fix detection to raise warning
b1c7f87 [Joris Van den Bossche] add whatsnew
925ce1b [Joris Van den Bossche] Update tests
0e10a9d [Graham R. Jeffries] remove read_excel kwd NotImplemented error, update documentation pandas-dev#11544
@jreback jreback mentioned this pull request Mar 27, 2017
Closed
jreback pushed a commit that referenced this pull request Mar 27, 2017
@grahamjeffries @jreback
Remove NotImplementedError for parse_dates keyword in read_excel
b36a461 
Rebase and update of PR #12051

Author: Joris Van den Bossche <[email protected]>
Author: Graham R. Jeffries <[email protected]>

This patch had conflicts when merged, resolved by
Committer: Jeff Reback <[email protected]>

Closes #14326 from jorisvandenbossche/pr/12051 and squashes the following commits:

0b65a7a [Joris Van den Bossche] update wording
656ec44 [Joris Van den Bossche] Fix detection to raise warning
b1c7f87 [Joris Van den Bossche] add whatsnew
925ce1b [Joris Van den Bossche] Update tests
0e10a9d [Graham R. Jeffries] remove read_excel kwd NotImplemented error, update documentation #11544
jreback pushed a commit that referenced this pull request Mar 27, 2017
@grahamjeffries @jreback
Remove NotImplementedError for parse_dates keyword in read_excel
7e43c78 
Rebase and update of PR #12051

Author: Joris Van den Bossche <[email protected]>
Author: Graham R. Jeffries <[email protected]>

This patch had conflicts when merged, resolved by
Committer: Jeff Reback <[email protected]>

Closes #14326 from jorisvandenbossche/pr/12051 and squashes the following commits:

0b65a7a [Joris Van den Bossche] update wording
656ec44 [Joris Van den Bossche] Fix detection to raise warning
b1c7f87 [Joris Van den Bossche] add whatsnew
925ce1b [Joris Van den Bossche] Update tests
0e10a9d [Graham R. Jeffries] remove read_excel kwd NotImplemented error, update documentation #11544
mattip pushed a commit to mattip/pandas that referenced this pull request Apr 3, 2017
@grahamjeffries @mattip
Remove NotImplementedError for parse_dates keyword in read_excel
40b4e24 
Rebase and update of PR pandas-dev#12051

Author: Joris Van den Bossche <[email protected]>
Author: Graham R. Jeffries <[email protected]>

This patch had conflicts when merged, resolved by
Committer: Jeff Reback <[email protected]>

Closes pandas-dev#14326 from jorisvandenbossche/pr/12051 and squashes the following commits:

0b65a7a [Joris Van den Bossche] update wording
656ec44 [Joris Van den Bossche] Fix detection to raise warning
b1c7f87 [Joris Van den Bossche] add whatsnew
925ce1b [Joris Van den Bossche] Update tests
0e10a9d [Graham R. Jeffries] remove read_excel kwd NotImplemented error, update documentation pandas-dev#11544
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
IO Excel read_excel, to_excel
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@grahamjeffries @jorisvandenbossche @jreback