-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG: Series.asof fails for all NaN Series (GH15713) #15758
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
Changes from 10 commits
676a4e5
c78d687
4e26ab8
17d1d77
89fb6cf
6b745af
70c958f
3f9c7fd
04b7306
a080b9b
7448b96
b8f078a
af9a29b
bb63964
0765108
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3972,6 +3972,16 @@ def asof(self, where, subset=None): | |
where = Index(where) if is_list else Index([where]) | ||
|
||
nulls = self.isnull() if is_series else self[subset].isnull().any(1) | ||
if nulls.all(): | ||
if is_series: | ||
return pd.Series(np.nan, index=where) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is not correct; should have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought about that, @jreback , but when I experimented with a non-null series, I saw that it has no name. I.e.:
returns
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and that not correct. we always want to propogate the names. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, let me write the test case and fix for nan and non-nan inputs There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jreback done here.. working on the request below, on simplifying the code |
||
else: | ||
if is_list: | ||
return pd.DataFrame(np.nan, index=where, | ||
columns=self.columns) | ||
else: | ||
return pd.Series(np.nan, index=[where]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So this should just be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done.. The test passes without name - no need to set it - index=self.columns is enough. Thanks @jorisvandenbossche There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is because you wrote your test without it, then of course it will pass it without it. As I showed before with this simple example of the current behaviour
you can clearly see that the name is the actual value you passed to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. my bad, sorry... just fixed it, it should work now... thanks again @jorisvandenbossche |
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see if you can simplify this logic a bit (maybe set the name where There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hey @jreback , I made a small simplification, pls check if that's ok... if it's ok, now I think everything is good to go |
||
locs = self.index.asof_locs(where, ~(nulls.values)) | ||
|
||
# mask the missing | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,22 +4,19 @@ | |
from pandas import (DataFrame, date_range, Timestamp, Series, | ||
to_datetime) | ||
|
||
from pandas.util.testing import assert_frame_equal, assert_series_equal | ||
import pandas.util.testing as tm | ||
|
||
from .common import TestData | ||
|
||
|
||
class TestFrameAsof(TestData, tm.TestCase): | ||
|
||
def setUp(self): | ||
self.N = N = 50 | ||
rng = date_range('1/1/1990', periods=N, freq='53s') | ||
self.rng = date_range('1/1/1990', periods=N, freq='53s') | ||
self.df = DataFrame({'A': np.arange(N), 'B': np.arange(N)}, | ||
index=rng) | ||
index=self.rng) | ||
|
||
def test_basic(self): | ||
|
||
df = self.df.copy() | ||
df.loc[15:30, 'A'] = np.nan | ||
dates = date_range('1/1/1990', periods=self.N * 3, | ||
|
@@ -39,7 +36,6 @@ def test_basic(self): | |
self.assertTrue((rs == 14).all(1).all()) | ||
|
||
def test_subset(self): | ||
|
||
N = 10 | ||
rng = date_range('1/1/1990', periods=N, freq='53s') | ||
df = DataFrame({'A': np.arange(N), 'B': np.arange(N)}, | ||
|
@@ -51,19 +47,19 @@ def test_subset(self): | |
# with a subset of A should be the same | ||
result = df.asof(dates, subset='A') | ||
expected = df.asof(dates) | ||
assert_frame_equal(result, expected) | ||
tm.assert_frame_equal(result, expected) | ||
|
||
# same with A/B | ||
result = df.asof(dates, subset=['A', 'B']) | ||
expected = df.asof(dates) | ||
assert_frame_equal(result, expected) | ||
tm.assert_frame_equal(result, expected) | ||
|
||
# B gives self.df.asof | ||
result = df.asof(dates, subset='B') | ||
expected = df.resample('25s', closed='right').ffill().reindex(dates) | ||
expected.iloc[20:] = 9 | ||
|
||
assert_frame_equal(result, expected) | ||
tm.assert_frame_equal(result, expected) | ||
|
||
def test_missing(self): | ||
# GH 15118 | ||
|
@@ -75,9 +71,35 @@ def test_missing(self): | |
result = df.asof('1989-12-31') | ||
|
||
expected = Series(index=['A', 'B'], name=Timestamp('1989-12-31')) | ||
assert_series_equal(result, expected) | ||
tm.assert_series_equal(result, expected) | ||
|
||
result = df.asof(to_datetime(['1989-12-31'])) | ||
expected = DataFrame(index=to_datetime(['1989-12-31']), | ||
columns=['A', 'B'], dtype='float64') | ||
assert_frame_equal(result, expected) | ||
tm.assert_frame_equal(result, expected) | ||
|
||
def test_all_nans(self): | ||
# GH 15713 | ||
# DataFrame is all nans | ||
result = DataFrame([np.nan]).asof([0]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. try these with non-defualt indexes and see what happens (your test will break) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed, and also, when you have a DataFrame with multiple columns, those columns should be preserved in the result There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
expected = DataFrame([np.nan]) | ||
tm.assert_frame_equal(result, expected) | ||
|
||
# testing non-default indexes, multiple inputs | ||
dates = date_range('1/1/1990', periods=self.N * 3, freq='25s') | ||
result = DataFrame(np.nan, index=self.rng, columns=['A']).asof(dates) | ||
expected = DataFrame(np.nan, index=dates, columns=['A']) | ||
tm.assert_frame_equal(result, expected) | ||
|
||
# testing multiple columns | ||
dates = date_range('1/1/1990', periods=self.N * 3, freq='25s') | ||
result = DataFrame(np.nan, index=self.rng, | ||
columns=['A', 'B', 'C']).asof(dates) | ||
expected = DataFrame(np.nan, index=dates, columns=['A', 'B', 'C']) | ||
tm.assert_frame_equal(result, expected) | ||
|
||
# testing scalar input | ||
date = date_range('1/1/1990', periods=self.N * 3, freq='25s')[0] | ||
result = DataFrame(np.nan, index=self.rng, columns=['A']).asof(date) | ||
expected = Series(np.nan, index=[date]) | ||
tm.assert_series_equal(result, expected) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -148,3 +148,24 @@ def test_errors(self): | |
s = Series(np.random.randn(N), index=rng) | ||
with self.assertRaises(ValueError): | ||
s.asof(s.index[0], subset='foo') | ||
|
||
def test_all_nans(self): | ||
# GH 15713 | ||
# series is all nans | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add the issue number as a comment There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
result = Series([np.nan]).asof([0]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you make this a separate test? (as it is not related to errors). Eg There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you also add a case not using zero as the argument? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
expected = Series([np.nan]) | ||
tm.assert_series_equal(result, expected) | ||
|
||
# testing non-default indexes | ||
N = 50 | ||
rng = date_range('1/1/1990', periods=N, freq='53s') | ||
|
||
dates = date_range('1/1/1990', periods=N * 3, freq='25s') | ||
result = Series(np.nan, index=rng).asof(dates) | ||
expected = Series(np.nan, index=dates) | ||
tm.assert_series_equal(result, expected) | ||
|
||
# testing scalar input | ||
date = date_range('1/1/1990', periods=N * 3, freq='25s')[0] | ||
result = Series(np.nan, index=rng).asof(date) | ||
assert isnull(result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI in the future, if you put the whatnew notes in a blank space in Bug Fixes (these are on purpose), you wont' get merge conflicts