Skip to content

BUG: Restrict DTA to 1D #27027

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

Merged
merged 4 commits into from
Jun 27, 2019
Merged

BUG: Restrict DTA to 1D #27027

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
73411ec
Restrict DTA to 1D
jbrockmendel Jun 25, 2019
e861530
address comments
jbrockmendel Jun 27, 2019
d59f41c
typo fixup
jbrockmendel Jun 27, 2019
8f99a00
rank comment
jbrockmendel Jun 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions pandas/core/algorithms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,12 @@ def _ensure_data(values, dtype=None):
dtype = values.dtype
else:
# Datetime
if values.ndim > 1:
Copy link
Contributor

Choose a reason for hiding this comment

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

what exactly hits this?

Copy link
Member Author

Choose a reason for hiding this comment

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

DataFrame.rank with all-datetime64 columns. #27015 has a terrible terrible hack instead of this 5-line workaround,.

Copy link
Contributor

Choose a reason for hiding this comment

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

we haven't reviewed #27015 not averse, just want to avoid hacks on hacks here

Copy link
Member Author

Choose a reason for hiding this comment

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

Yah, this is distinctly the non-hack solution.

# Avoid calling the DatetimeIndex constructor as it is 1D only
asi8 = values.view('i8')
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that ensure_data should only take 1d input at all times. Is there a case where it does not? (nb we shoul dprob document / type this)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this gets called with 2D values from DataFrame.rank.

Copy link
Member

Choose a reason for hiding this comment

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

Do you know if it is only rank? Because if so, it might be useful to add that as a comment for somebody later reading the code and wondering the same question where 2D things are passed to this.

dtype = values.dtype
return asi8, dtype, 'int64'

from pandas import DatetimeIndex
values = DatetimeIndex(values)
dtype = values.dtype
Expand Down
2 changes: 2 additions & 0 deletions pandas/core/arrays/datetimes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,8 @@ def __init__(self, values, dtype=_NS_DTYPE, freq=None, copy=False):
"ndarray, or Series or Index containing one of those."
)
raise ValueError(msg.format(type(values).__name__))
if values.ndim != 1:
raise ValueError("Only 1-dimensional input arrays are supported.")

if values.dtype == 'i8':
# for compat with datetime/timedelta/period shared methods,
Expand Down
5 changes: 5 additions & 0 deletions pandas/io/formats/format.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1273,6 +1273,11 @@ def format_percentiles(percentiles):

def _is_dates_only(values):
# return a boolean if we are only dates (and don't have a timezone)
if isinstance(values, np.ndarray) and values.ndim > 1:
Copy link
Member

Choose a reason for hiding this comment

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

In which case do you run into this?
(I was assuming the format_array is working column by column)

Copy link
Member Author

Choose a reason for hiding this comment

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

Per above, this is hit with 2D ndarray inputs, which ATM are incorrectly accepted but will now raise.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but my question is: when is this actually hit with 2D ndarray input?

Copy link
Contributor

Choose a reason for hiding this comment

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

same question as @jorisvandenbossche

Copy link
Member Author

Choose a reason for hiding this comment

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

Just added an assertion for 1D-ness (in master) and the first test that failed is effectively:

pd.DataFrame({"A": pd.date_range('2016-01-01', periods=3)}).to_csv()

Copy link
Member

Choose a reason for hiding this comment

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

OK. I would rather do a ravel in the code calling it:

pandas/pandas/core/internals/blocks.py

Lines 2171 to 2175 in f0919f2

fmt = _get_format_datetime64_from_values(values, date_format)
result = tslib.format_array_from_datetime(
i8values.ravel(), tz=getattr(self.values, 'tz', None),
format=fmt, na_rep=na_rep).reshape(i8values.shape)

as it is also done for the actual formatting function right below.

In fact, this is also kind of a bug in our formatting. As the formatting should be done column by column (the frequency of one column should not influence the formatting of another column)

Copy link
Member Author

Choose a reason for hiding this comment

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

OK. I think is_dates_only is only called with non-ravelled data in one place, so I can move the maybe ravel there and put an assertion in is_dates_only. Is there anything else that should go along with that?

Copy link
Contributor

Choose a reason for hiding this comment

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

did you update this?

# We don't actaully care about the order of values, and DatetimeIndex
# only accepts 1D values
values = values.ravel()

values = DatetimeIndex(values)
if values.tz is not None:
return False
Expand Down
12 changes: 12 additions & 0 deletions pandas/tests/arrays/test_datetimes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,18 @@


class TestDatetimeArrayConstructor:

def test_only_1dim_accepted(self):
arr = np.array([0, 1, 2, 3], dtype='M8[h]').astype('M8[ns]')

with pytest.raises(ValueError, match="Only 1-dimensional"):
# 2-dim
DatetimeArray(arr.reshape(2, 2))
Copy link
Member

Choose a reason for hiding this comment

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

To be clear: this already fails currently, right? You are mainly catching the error early / providing a better error message ?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this is currently accepted incorrectly.

Copy link
Member

Choose a reason for hiding this comment

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

With latest master:

In [15]: pd.__version__
Out[15]: '0.25.0.dev0+791.gf0919f272'

In [16]: arr = np.array([0, 1, 2, 3], dtype='M8[h]').astype('M8[ns]') 

In [17]: pd.arrays.DatetimeArray(arr.reshape(2, 2)) 
...
~/scipy/pandas/pandas/_libs/tslibs/conversion.pyx in pandas._libs.tslibs.conversion.convert_to_tsobject()

TypeError: Cannot convert input [['1970-01-01T00:00:00.000000000' '1970-01-01T01:00:00.000000000']] of type <class 'numpy.ndarray'> to Timestamp

Copy link
Member Author

Choose a reason for hiding this comment

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

Try changing [17] to res = pd.arrays.DatetimeArray(arr.reshape(2, 2)). I'm pretty sure that error is coming from an attempt to call __repr__.

Copy link
Member

Choose a reason for hiding this comment

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

ah .. yes :-)


with pytest.raises(ValueError, match="Only 1-dimensional"):
# 0-dim
DatetimeArray(arr[[0]].squeeze())

def test_freq_validation(self):
# GH#24623 check that invalid instances cannot be created with the
# public constructor
Expand Down