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

BUG: Fix to_dict() problem when using datetime DataFrame #11247 #11327

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@ethanluoyc
Contributor

ethanluoyc commented Oct 14, 2015

closes #11247

This is my first time contributing through PR so forgive me if I make any silly mistake.
The issue can be referred to in GH11247
I followed @jreback 's suggestion to box things up in to_dict() but I am not sure if this is what he means because I do not know exactly when _maybe_datetime() should be used. Initially I tried to tweak internals.py but it fails.

Let me know how I can improve from here. Hope I can really learn from my first PR.

@jreback

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Oct 14, 2015

Contributor

see guidelines here: http://pandas.pydata.org/pandas-docs/stable/contributing.html

dont' turn on PEP changes for your editor.

Contributor

jreback commented Oct 14, 2015

see guidelines here: http://pandas.pydata.org/pandas-docs/stable/contributing.html

dont' turn on PEP changes for your editor.

@ethanluoyc

This comment has been minimized.

Show comment
Hide comment
@ethanluoyc

ethanluoyc Oct 14, 2015

Contributor

I think I forgot to fetch to update my local master before my first commit to this PR. Somehow I messed up is it true?

Contributor

ethanluoyc commented Oct 14, 2015

I think I forgot to fetch to update my local master before my first commit to this PR. Somehow I messed up is it true?

@jreback

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Oct 14, 2015

Contributor

add tests! (ideally for each one of the orients if we dont' have already).

Contributor

jreback commented Oct 14, 2015

add tests! (ideally for each one of the orients if we dont' have already).

@ethanluoyc

This comment has been minimized.

Show comment
Hide comment
@ethanluoyc

ethanluoyc Oct 14, 2015

Contributor

which test case shall I add in? I am new to this :)

On 15 Oct 2015, at 2:50 AM, Jeff Reback notifications@github.com wrote:

add tests! (ideally for each one of the orients if we dont' have already).


Reply to this email directly or view it on GitHub #11327 (comment).

Contributor

ethanluoyc commented Oct 14, 2015

which test case shall I add in? I am new to this :)

On 15 Oct 2015, at 2:50 AM, Jeff Reback notifications@github.com wrote:

add tests! (ideally for each one of the orients if we dont' have already).


Reply to this email directly or view it on GitHub #11327 (comment).

@ethanluoyc

This comment has been minimized.

Show comment
Hide comment
@ethanluoyc

ethanluoyc Oct 15, 2015

Contributor

@jreback I have added tests:)

Contributor

ethanluoyc commented Oct 15, 2015

@jreback I have added tests:)

def test_to_dict_timestamp(self):
# GH11247
tsmp = Timestamp('20130101')
test_data = DataFrame({'A': [tsmp, tsmp], 'B': [tsmp, tsmp]})

This comment has been minimized.

@jreback

jreback Oct 15, 2015

Contributor

can you make this a mixed frame ,e.g. add a float and a string say. obviously need to adjust the tests as well.

@jreback

jreback Oct 15, 2015

Contributor

can you make this a mixed frame ,e.g. add a float and a string say. obviously need to adjust the tests as well.

This comment has been minimized.

@ethanluoyc

ethanluoyc Oct 16, 2015

Contributor

I adjusted the tests and now it tests both mixed and single dtypes

@ethanluoyc

ethanluoyc Oct 16, 2015

Contributor

I adjusted the tests and now it tests both mixed and single dtypes

@jreback

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Oct 15, 2015

Contributor

pls add a whatsnew note for 0.17.1 in bug fixes

Contributor

jreback commented Oct 15, 2015

pls add a whatsnew note for 0.17.1 in bug fixes

@jreback jreback added this to the 0.17.1 milestone Oct 15, 2015

@ethanluoyc

This comment has been minimized.

Show comment
Hide comment
@ethanluoyc

ethanluoyc Oct 16, 2015

Contributor

Just asking what I should do if I merge updates locally with my branch and need to merge the 2 commits into one..


Sent from Mailbox

On Fri, Oct 16, 2015 at 6:25 AM, Jeff Reback notifications@github.com
wrote:

pls add a whatsnew note for 0.17.1 in bug fixes

Reply to this email directly or view it on GitHub:
#11327 (comment)

Contributor

ethanluoyc commented Oct 16, 2015

Just asking what I should do if I merge updates locally with my branch and need to merge the 2 commits into one..


Sent from Mailbox

On Fri, Oct 16, 2015 at 6:25 AM, Jeff Reback notifications@github.com
wrote:

pls add a whatsnew note for 0.17.1 in bug fixes

Reply to this email directly or view it on GitHub:
#11327 (comment)

BUG: Fix to_dict() problem when using only datetime #11247
Fix a bug where to_dict() does not return Timestamp when there is only
datetime dtype present.
@jreback

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Oct 16, 2015

Contributor

merged via 89b4e5b

thanks!

Contributor

jreback commented Oct 16, 2015

merged via 89b4e5b

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment