Skip to content

BUG: Fix pd.Timedelta(None) to return NaT. #13723

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

BUG: Fix pd.Timedelta(None) to return NaT. #13723

wants to merge 1 commit into from

Conversation

yui-knk
Copy link
Contributor

@yui-knk yui-knk commented Jul 20, 2016 •
edited
Loading

  • closes #xxxx
  • tests added / passed
  • passes git diff upstream/master | flake8 --diff
  • whatsnew entry

Before this commit pd.Timedelta raised ValueError
when None is passed.
This behavior is inconsistent because

  • pd.Timestamp(None) and pd.Period(None) return NaT
  • pd.Timedelta returns NaT if '', 'nat', 'NAT' or np.nan
    is passed

This PR is related to #13687 (comment)

@jreback jreback added the Timedelta Timedelta data type label Jul 20, 2016
@jreback jreback added this to the 0.19.0 milestone Jul 20, 2016
@codecov-io
Copy link

codecov-io commented Jul 20, 2016 •
edited
Loading

Current coverage is 84.57% (diff: 100%)

No coverage report found for master at 6efd743.

Powered by Codecov. Last update 6efd743...5e29ad9

jorisvandenbossche
jorisvandenbossche reviewed Jul 20, 2016
View reviewed changes
doc/source/whatsnew/v0.19.0.txt
@@ -667,3 +667,4 @@ Bug Fixes
- Bug where ``pd.read_gbq()`` could throw ``ImportError: No module named discovery`` as a result of a naming conflict with another python package called apiclient (:issue:`13454`)
- Bug in ``Index.union`` returns an incorrect result with a named empty index (:issue:`13432`)
- Bugs in ``Index.difference`` and ``DataFrame.join`` raise in Python3 when using mixed-integer indexes (:issue:`13432`, :issue:`12814`)
- Bug in ``pd.Timedelta(None)`` raises ``ValueError``. This is different from ``pd.Timestamp(None)`` and ``pd.Period(None)``.
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a reference to an issue number? (see how the other entries do it, you can use the PR number)

Copy link
Member

Choose a reason for hiding this comment

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

Period(None) is also changed in this version (#13609), thus no need to refer here.

@sinhrks sinhrks mentioned this pull request Jul 21, 2016
Merged
4 tasks
@jorisvandenbossche
Copy link
Member

Actually, regarding no arguments, Timestamp raises in that case:

In [2]: pd.Timestamp(None)
Out[2]: NaT

In [3]: pd.Timestamp()
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-3-384f27778dc4> in <module>()
----> 1 pd.Timestamp()

pandas\tslib.pyx in pandas.tslib.Timestamp.__new__ (pandas\tslib.c:9160)()

TypeError: __new__() takes at least 2 positional arguments (1 given)

So not sure if we should let it here return NaT.

@jreback
Copy link
Contributor

jreback commented Jul 21, 2016 •
edited
Loading

no pd.Timestamp(), .Timedelta(), .Period() are errors, passing None is ok though as better to be explicit.

@yui-knk
Copy link
Contributor Author

yui-knk commented Jul 24, 2016

Rebased and updated.

@jreback
Copy link
Contributor

jreback commented Jul 24, 2016

pls add a whatsnew note.

FYI, PR's always need notes & tests & sometimes doc updates. These are often much more effort, than the actual code change. Esp for tests these need to be done upfront.

@yui-knk
Copy link
Contributor Author

yui-knk commented Jul 24, 2016

Sorry I misunderstood what sinhrks said. I added a whatsnew note.
Thanks for your advice.

@yui-knk
BUG: Fix pd.Timedelta(None) to return NaT.
46a0f38 
Before this commit pd.Timedelta raised ValueError
when None is passed.
This behavior is inconsistent because

* pd.Timestamp(None) and pd.Period(None) return NaT
* pd.Timedelta returns NaT if '', 'nat', 'NAT' or np.nan
  is passed
jreback
jreback reviewed Jul 24, 2016
View reviewed changes
doc/source/whatsnew/v0.19.0.txt
@@ -755,3 +755,4 @@ Bug Fixes
- Bug where ``pd.read_gbq()`` could throw ``ImportError: No module named discovery`` as a result of a naming conflict with another python package called apiclient (:issue:`13454`)
- Bug in ``Index.union`` returns an incorrect result with a named empty index (:issue:`13432`)
- Bugs in ``Index.difference`` and ``DataFrame.join`` raise in Python3 when using mixed-integer indexes (:issue:`13432`, :issue:`12814`)
- Bug in ``pd.Timedelta(None)`` raises ``ValueError``. This is different from ``pd.Timestamp(None)`` (:issue:`13687`)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is different from pd.Timestamp(None)

what does that mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this commit aim to keep consistency with other Time related class.
So I added behavior of other class.

Copy link
Member

Choose a reason for hiding this comment

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

How about:

  • Bug in pd.Timedelta(None) raises ValueError rather than returning pd.NaT as the same as pd.Timestamp(None)

pls wait for @jreback and @jorisvandenbossche comments as i'm not a native:)

@jreback jreback closed this in c1d8c09 Jul 24, 2016
jreback added a commit that referenced this pull request Jul 24, 2016
@jreback
DOC: whatsnew for #13723
2c047d4
@jreback
Copy link
Contributor

jreback commented Jul 24, 2016

thanks, whatsnew here

@yui-knk yui-knk deleted the timedelta_none branch July 24, 2016 16:03
@yui-knk
Copy link
Contributor Author

yui-knk commented Jul 24, 2016

😍 Thanks!

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
Timedelta Timedelta data type
Projects
None yet
Milestone
0.19.0
Development

Successfully merging this pull request may close these issues.
5 participants
@yui-knk @codecov-io @jorisvandenbossche @jreback @sinhrks