Skip to content

Timestamp replace compatibility #15248

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 7 commits into from
Closed

Timestamp replace compatibility #15248

wants to merge 7 commits into from

Conversation

sdementen
Copy link
Contributor

jreback
jreback reviewed Jan 27, 2017
View reviewed changes
doc/source/whatsnew/v0.20.0.txt
@@ -416,6 +416,7 @@ Performance Improvements
Bug Fixes
~~~~~~~~~

- Bug in ``Timestamp.replace`` wrong exception raised when wrong argument name given (:issue:`15240`)
Copy link
Contributor

Choose a reason for hiding this comment

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

say should be TypeError

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done and left in bug fixes as it is a bug introduced by a previous change (not really an excepted API change)

@jreback
Copy link
Contributor

jreback commented Jan 27, 2017

lgtm. ping on green.

@jreback jreback added Bug Error Reporting Incorrect or improved errors from pandas Datetime Datetime data dtype and removed Bug labels Jan 27, 2017
@jreback jreback mentioned this pull request Jan 28, 2017
Closed
3 tasks
@sdementen
Copy link
Contributor Author

sdementen commented Jan 28, 2017 •
edited
Loading

@jreback I am bit puzzled by the errors in the tests. I see issues but I think they are unrelated to my changes. Essentially, I have a) change a signature of function, b) renamed an internal variable (tzinfo) and c) adapted some internal code) but I get error about infinite recursion in some totally unrelated tests (but I may be wrong in my interpretation)

sdementen and others added 3 commits January 28, 2017 07:03
@sdementen @jreback
fix for issue pandas-dev#15240
9455ee9 
replace **kwd argument to explicit argument to raise TypeError when wrong argument name is given
@sdementen @jreback
improve description of whatsnew item
52b6c2c
@jreback
use tzinfo=object to accept None as a default parameter
e70e75a
@jreback jreback force-pushed the timestamp-replace-compatibility branch from 4cf0ae0 to e70e75a Compare January 28, 2017 12:40
@jreback
Copy link
Contributor

jreback commented Jan 28, 2017

@sdementen I pushed a fix to your branch. This is actually a tricky thing, .replace accepts None as an actual value for tzinfo. So you need to distinguish between not passing it and passing it with a value of None. The trick is to default it like tzinfo=object, then check if tzinfo is object, means it was not passed.

This shows up in the way it did because .replace is used a lot, for example, to reset the time zone on objects, e.g. Timestamp.replace(tzinfo=None) is common in dateutil

@jreback jreback added this to the 0.20.0 milestone Jan 28, 2017
@jreback jreback closed this in 66d8c41 Jan 28, 2017
@jreback
Copy link
Contributor

jreback commented Jan 28, 2017

thanks @sdementen !

@sdementen
Copy link
Contributor Author

Thanks to you for your support in this PR !

@sdementen
Copy link
Contributor Author

BTW, the replace function when used to change the time (not the tzinfo part), leads sometimes to inconsistent times when dst is involved in the case the replaced date cross the dst (and so should have his offset changed).
Is this a bug ? Or a difficult to solve limit ? Is the introduction of the fold argument a part of a future solution ?
If it is a bug not yet identified, i can add an issue

@jreback
Copy link
Contributor

jreback commented Jan 28, 2017

@sdementen you would have to show an example. .replace replaces current times; however it is possible to then generate an invalid time. datetime.datetime doesn't handle this at all. Timestamp does to some extant. Look thru open issues and you will see a couple that are out there.

AnkurDedania pushed a commit to AnkurDedania/pandas that referenced this pull request Mar 21, 2017
@sdementen @AnkurDedania
ERR: Timestamp replace compatibility
feaa7cd 
closes pandas-dev#15240

Author: Sébastien de Menten <[email protected]>
Author: sdementen <[email protected]>
Author: Jeff Reback <[email protected]>

Closes pandas-dev#15248 from sdementen/timestamp-replace-compatibility and squashes the following commits:

e70e75a [Jeff Reback] use tzinfo=object to accept None as a default parameter
52b6c2c [Sébastien de Menten] improve description of whatsnew item
9455ee9 [sdementen] fix for issue pandas-dev#15240
1a40777 [Sébastien de Menten] update what's new with bug fix
5263054 [Sébastien de Menten] fix typos + update doc
d5c7b0a [Sébastien de Menten] update exception type raised when wrong argument to Timestamp.replace
2ac141a [sdementen] fix for issue pandas-dev#15240
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jreback jreback jreback left review comments

Assignees
No one assigned
Labels
Datetime Datetime data dtype Error Reporting Incorrect or improved errors from pandas
Projects
None yet
Milestone
0.20.0
Development

Successfully merging this pull request may close these issues.

Timestamp.replace raises ValueError instead of TypeError when given wrong argument
2 participants
@sdementen @jreback