Skip to content

BUG GH23451 Allow setting date in string index for Series #23495

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 4 commits into from
Nov 6, 2018

Conversation

JustinZhengBC
Copy link
Contributor

Prevents string indices from turning into lists of characters when setting a date.

@pep8speaks
Copy link

Hello @JustinZhengBC! Thanks for submitting the PR.

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

needs a whatsnew note as well

@@ -947,6 +947,9 @@ def _set_with(self, key, value):
except Exception:
pass

if isinstance(key, str):
key = [key]

if not isinstance(key, (list, Series, np.ndarray, Series)):
Copy link
Contributor

Choose a reason for hiding this comment

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

Series is here twice :< can you remove

Copy link
Contributor

Choose a reason for hiding this comment

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

this can be an elseif

@@ -947,6 +947,9 @@ def _set_with(self, key, value):
except Exception:
pass

if isinstance(key, str):
Copy link
Contributor

Choose a reason for hiding this comment

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

you might be able to make this is_scalar

@@ -548,3 +548,9 @@ def test_minmax_nat_series(self, nat):
def test_minmax_nat_dataframe(self, nat):
assert nat.min()[0] is pd.NaT
assert nat.max()[0] is pd.NaT

def test_set_dt_with_string_index(self):
from datetime import date
Copy link
Contributor

Choose a reason for hiding this comment

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

imports go at the top

def test_set_dt_with_string_index(self):
from datetime import date
x = pd.Series([1, 2, 3], index=['Date', 'b', 'other'])
x.Date = date.today()
Copy link
Contributor

Choose a reason for hiding this comment

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

can you try this both as (parametrize)
x.Date = and x['Date'] =

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 can't seem to figure out how to parametrize both these cases, as "Date" is being used in different contexts. What would be the variable(s) I parametrize and how would I use it with x to cover both cases?

@jreback jreback added Datetime Datetime data dtype Indexing Related to indexing on series/frames, not to indexes themselves labels Nov 4, 2018
@codecov
Copy link

codecov bot commented Nov 4, 2018

Codecov Report

Merging #23495 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #23495      +/-   ##
==========================================
- Coverage   92.25%   92.25%   -0.01%     
==========================================
  Files         161      161              
  Lines       51176    51178       +2     
==========================================
  Hits        47214    47214              
- Misses       3962     3964       +2
Flag Coverage Δ
#multiple 90.63% <100%> (-0.01%) ⬇️
#single 42.28% <0%> (-0.01%) ⬇️
Impacted Files Coverage Δ
pandas/core/series.py 93.7% <100%> (-0.18%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 54982c2...12d2f8a. Read the comment docs.

@mroeschke
Copy link
Member

Can you see if this fixes #12862 as well?

@JustinZhengBC
Copy link
Contributor Author

@mroeschke it fixes the issue of strings being split into lists, but that bug still persists because for tz-aware timestamps, the data is stored in a DateTimeIndex, whereas almost every other data format is stored in an ndarray, and setting an item directly requires an ndarray to be passed to the backend function. After this, I can do a quick PR to make the [] operator work just like using .loc, but it would still lose the timezone data after adding a new item. @jreback is that okay or should the problem be fixed closer to the root cause?

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

small comments, ping on green.

@@ -1115,6 +1115,7 @@ Datetimelike
- Bug in :func:`DataFrame.combine` with datetimelike values raising a TypeError (:issue:`23079`)
- Bug in :func:`date_range` with frequency of ``Day`` or higher where dates sufficiently far in the future could wrap around to the past instead of raising ``OutOfBoundsDatetime`` (:issue:`14187`)
- Bug in :class:`PeriodIndex` with attribute ``freq.n`` greater than 1 where adding a :class:`DateOffset` object would return incorrect results (:issue:`23215`)
- Bug in :class:`Series` that interpreted string indices as lists of characters when setting datetimelike values
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add the issue number

@@ -548,3 +548,10 @@ def test_minmax_nat_series(self, nat):
def test_minmax_nat_dataframe(self, nat):
assert nat.min()[0] is pd.NaT
assert nat.max()[0] is pd.NaT

def test_set_dt_with_string_index(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

rename to test_setitem_with_string_index

@jreback jreback added this to the 0.24.0 milestone Nov 5, 2018
@jreback
Copy link
Contributor

jreback commented Nov 6, 2018

lgtm. ping on green.

@JustinZhengBC
Copy link
Contributor Author

@jreback green now

@jreback jreback merged commit c95bfd1 into pandas-dev:master Nov 6, 2018
@jreback
Copy link
Contributor

jreback commented Nov 6, 2018

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Datetime Datetime data dtype Indexing Related to indexing on series/frames, not to indexes themselves
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't put date in Series if index is a string longer than 1 character
4 participants