Skip to content

Fix docstrings, error messages #19672

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

Closed
wants to merge 7 commits into from
Closed

Fix docstrings, error messages #19672

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ddb78de
Fic docstrings, make error messages class-agnostic
jbrockmendel Feb 13, 2018
08d575e
revert some less obvious changes
jbrockmendel Feb 13, 2018
f0d203f
one more formatter
jbrockmendel Feb 13, 2018
a7775ea
make inheritance more transparent
jbrockmendel Feb 13, 2018
c779f64
improve DTI/TDI shift docstring
jbrockmendel Feb 13, 2018
5c2868a
fix unreached NameError
jbrockmendel Feb 13, 2018
a39b22a
Merge branch 'master' into array_prelims
jbrockmendel Feb 16, 2018
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
20 changes: 11 additions & 9 deletions pandas/core/indexes/datetimelike.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -627,9 +627,9 @@ def _convert_scalar_indexer(self, key, kind=None):
._convert_scalar_indexer(key, kind=kind))

def _add_datelike(self, other):
raise TypeError("cannot add {0} and {1}"
.format(type(self).__name__,
type(other).__name__))
raise TypeError("cannot add {cls} and {typ}"
.format(cls=type(self).__name__,
typ=type(other).__name__))

def _sub_datelike(self, other):
raise com.AbstractMethodError(self)
Expand Down Expand Up @@ -670,8 +670,9 @@ def __add__(self, other):
elif isinstance(self, TimedeltaIndex) and isinstance(other, Index):
if hasattr(other, '_add_delta'):
return other._add_delta(self)
raise TypeError("cannot add TimedeltaIndex and {typ}"
.format(typ=type(other)))
raise TypeError("cannot add {cls} and {typ}"
.format(cls=type(self).__name__,
typ=type(other).__name__))
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure this is an improvement in robustness, as if series would use the same code, this 'cls' would become either "Series" or "TimedeltaArray", and "Series" is not that informative, and "TimedeltaArray" should not bubble up to user when using a Series (that would be an internal implementation detail).

Copy link
Member

Choose a reason for hiding this comment

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

The above is always a TimedeltaIndex given the elif statement above

elif is_integer(other):
return self.shift(other)
elif isinstance(other, (datetime, np.datetime64)):
Expand Down Expand Up @@ -705,8 +706,9 @@ def __sub__(self, other):
return self._sub_offset_array(other)
elif isinstance(self, TimedeltaIndex) and isinstance(other, Index):
if not isinstance(other, TimedeltaIndex):
raise TypeError("cannot subtract TimedeltaIndex and {typ}"
.format(typ=type(other).__name__))
raise TypeError("cannot subtract {cls} and {typ}"
.format(cls=type(self).__name__,
typ=type(other).__name__))
Copy link
Member

Choose a reason for hiding this comment

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

same here

return self._add_delta(-other)
elif isinstance(other, DatetimeIndex):
return self._sub_datelike(other)
Expand Down Expand Up @@ -794,7 +796,7 @@ def isin(self, values):

def shift(self, n, freq=None):
"""
Specialized shift which produces a DatetimeIndex
Specialized shift which produces a %(klass)s
Copy link
Member

Choose a reason for hiding this comment

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

Instead of doing the overriding in the subclass just to fix this name, we can maybe reword the docstring to be clear enough?
Eg mention it works for Datetime- and TimedeltaIndex?

Copy link
Member Author

Choose a reason for hiding this comment

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

That was the idea in the earlier version. I'm open to suggestions.


Parameters
----------
Expand All @@ -804,7 +806,7 @@ def shift(self, n, freq=None):

Returns
-------
shifted : DatetimeIndex
shifted : %(klass)s
"""
if freq is not None and freq != self.freq:
if isinstance(freq, compat.string_types):
Expand Down
39 changes: 23 additions & 16 deletions pandas/core/indexes/datetimes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,15 +106,15 @@ def _dt_index_cmp(opname, cls, nat_result=False):
"""

def wrapper(self, other):
func = getattr(super(DatetimeIndex, self), opname)
binop = getattr(Index, opname)

if isinstance(other, (datetime, compat.string_types)):
if isinstance(other, datetime):
# GH#18435 strings get a pass from tzawareness compat
self._assert_tzawareness_compat(other)

other = _to_m8(other, tz=self.tz)
result = func(other)
result = binop(self, other)
Copy link
Member

Choose a reason for hiding this comment

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

why this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because DatetimeIndex.__mro__ has 12 entries, so with the standard super usage it isn't obvious where the method is defined.

if isna(other):
result.fill(nat_result)
else:
Expand All @@ -134,7 +134,7 @@ def wrapper(self, other):
if is_datetimelike(other):
self._assert_tzawareness_compat(other)

result = func(np.asarray(other))
result = binop(self, np.asarray(other))
result = com._values_from_object(result)

if isinstance(other, Index):
Expand Down Expand Up @@ -856,18 +856,19 @@ def _add_datelike(self, other):
# adding a timedeltaindex to a datetimelike
if other is libts.NaT:
return self._nat_new(box=True)
raise TypeError("cannot add {0} and {1}"
.format(type(self).__name__,
type(other).__name__))
raise TypeError("cannot add {cls} and {typ}"
.format(cls=type(self).__name__,
typ=type(other).__name__))

def _sub_datelike(self, other):
# subtract a datetime from myself, yielding a TimedeltaIndex
from pandas import TimedeltaIndex
if isinstance(other, DatetimeIndex):
# require tz compat
if not self._has_same_tz(other):
raise TypeError("DatetimeIndex subtraction must have the same "
"timezones or no timezones")
raise TypeError("{cls} subtraction must have the same "
Copy link
Member

Choose a reason for hiding this comment

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

Same here, this will always be subtraction of DatetimeIndexes ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is now, but one of the on-deck PRs will have ndarray[datetime64] go down this path too to fix:

dti = pd.date_range('2016-01-01', periods=3)
arr = np.array(['2015-01-01', '2015-02-02', '2015-03-03'], dtype='datetime64[ns]')
>>> dti - arr
DatetimeIndex(['1971-01-01', '1970-12-01', '1970-11-03'], dtype='datetime64[ns]', freq=None)

Copy link
Contributor

Choose a reason for hiding this comment

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

how about instead of using type(self).__name__ we make a little function which can format these. I agree with @jorisvandenbossche point here, in the case of an Index type the dtype is clear, but not for a Series.

Copy link
Member Author

Choose a reason for hiding this comment

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

This seems worthwhile but a pretty low priority. Closing for now.

"timezones or no timezones"
.format(cls=type(self).__name__))
result = self._sub_datelike_dti(other)
elif isinstance(other, (datetime, np.datetime64)):
other = Timestamp(other)
Expand All @@ -884,8 +885,9 @@ def _sub_datelike(self, other):
result = self._maybe_mask_results(result,
fill_value=libts.iNaT)
else:
raise TypeError("cannot subtract DatetimeIndex and {typ}"
.format(typ=type(other).__name__))
raise TypeError("cannot subtract {cls} and {typ}"
.format(cls=type(self).__name__,
typ=type(other).__name__))
Copy link
Member

Choose a reason for hiding this comment

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

same here?

return TimedeltaIndex(result, name=self.name, copy=False)

def _sub_datelike_dti(self, other):
Expand All @@ -901,6 +903,11 @@ def _sub_datelike_dti(self, other):
new_values[mask] = libts.iNaT
return new_values.view('i8')

@Substitution(klass='DatetimeIndex')
@Appender(DatetimeIndexOpsMixin.shift.__doc__)
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be a shared doc-string instead (and same for the other things you added). this is the existing pattern.

def shift(self, n, freq=None):
return super(DatetimeIndex, self).shift(n, freq)

def _maybe_update_attributes(self, attrs):
""" Update Index attributes (e.g. freq) depending on op """
freq = attrs.get('freq', None)
Expand Down Expand Up @@ -948,8 +955,8 @@ def _add_offset(self, offset):
return result

except NotImplementedError:
warnings.warn("Non-vectorized DateOffset being applied to Series "
"or DatetimeIndex", PerformanceWarning)
warnings.warn("Non-vectorized DateOffset being applied to {cls}"
.format(cls=type(self).__name__), PerformanceWarning)
return self.astype('O') + offset

def _add_offset_array(self, other):
Expand All @@ -960,8 +967,8 @@ def _add_offset_array(self, other):
return self + other[0]
else:
warnings.warn("Adding/subtracting array of DateOffsets to "
"{} not vectorized".format(type(self)),
PerformanceWarning)
"{cls} not vectorized"
.format(cls=type(self).__name__), PerformanceWarning)
return self.astype('O') + np.array(other)
# TODO: This works for __add__ but loses dtype in __sub__

Expand All @@ -973,8 +980,8 @@ def _sub_offset_array(self, other):
return self - other[0]
else:
warnings.warn("Adding/subtracting array of DateOffsets to "
"{} not vectorized".format(type(self)),
PerformanceWarning)
"{cls} not vectorized"
.format(cls=type(self).__name__), PerformanceWarning)
res_values = self.astype('O').values - np.array(other)
return self.__class__(res_values, freq='infer')

Expand Down
44 changes: 28 additions & 16 deletions pandas/core/indexes/timedeltas.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,23 +59,25 @@ def _td_index_cmp(opname, cls, nat_result=False):
"""

def wrapper(self, other):
msg = "cannot compare a TimedeltaIndex with type {0}"
func = getattr(super(TimedeltaIndex, self), opname)
msg = "cannot compare a {cls} with type {typ}"
Copy link
Member

Choose a reason for hiding this comment

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

same here? self is always a TimedeltaIndex

Copy link
Member Author

Choose a reason for hiding this comment

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

TDI/DTI/PI have very similar implementations for these methods, some of which can be de-duplicated if we make these class-agnostic. More to the point, we want these messages to remain accurate if/when they get moved to the array level. Also in principle this could be subclassed and we'd like the name to come out right there too.

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 some tests which lock down these error messages? IOW the types that are supposed to appear

binop = getattr(Index, opname)
if _is_convertible_to_td(other) or other is NaT:
try:
other = _to_m8(other)
except ValueError:
# failed to parse as timedelta
raise TypeError(msg.format(type(other)))
result = func(other)
raise TypeError(msg.format(cls=type(self).__name__,
typ=type(other).__name__))
result = binop(self, other)
if isna(other):
result.fill(nat_result)
else:
if not is_list_like(other):
raise TypeError(msg.format(type(other)))
raise TypeError(msg.format(cls=type(self).__name__,
typ=type(other).__name__))

other = TimedeltaIndex(other).values
result = func(other)
result = binop(self, other)
result = com._values_from_object(result)

if isinstance(other, Index):
Expand Down Expand Up @@ -364,8 +366,9 @@ def _add_delta(self, delta):
# update name when delta is index
name = com._maybe_match_name(self, delta)
else:
raise TypeError("cannot add the type {0} to a TimedeltaIndex"
.format(type(delta)))
raise TypeError("cannot add the type {typ} to a {cls}"
.format(typ=type(delta).__name__,
cls=type(self).__name__))

result = TimedeltaIndex(new_values, freq='infer', name=name)
return result
Expand Down Expand Up @@ -409,7 +412,7 @@ def _add_datelike(self, other):
result = checked_add_with_arr(i8, other.value,
arr_mask=self._isnan)
result = self._maybe_mask_results(result, fill_value=iNaT)
return DatetimeIndex(result, name=self.name, copy=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

what changed 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.

It just got indented (next line in the diff) b/c this branch is never reached except for in the indented case.

return DatetimeIndex(result, name=self.name, copy=False)

def _sub_datelike(self, other):
# GH#19124 Timedelta - datetime is not in general well-defined.
Expand All @@ -418,7 +421,8 @@ def _sub_datelike(self, other):
if other is NaT:
return self._nat_new()
else:
raise TypeError("cannot subtract a datelike from a TimedeltaIndex")
raise TypeError("cannot subtract a datelike from a {cls}"
.format(cls=type(self).__name__))

def _add_offset_array(self, other):
# Array/Index of DateOffset objects
Expand All @@ -433,12 +437,14 @@ def _add_offset_array(self, other):
else:
from pandas.errors import PerformanceWarning
warnings.warn("Adding/subtracting array of DateOffsets to "
"{} not vectorized".format(type(self)),
"{cls} not vectorized"
.format(cls=type(self).__name__),
PerformanceWarning)
return self.astype('O') + np.array(other)
# TODO: This works for __add__ but loses dtype in __sub__
except AttributeError:
raise TypeError("Cannot add non-tick DateOffset to TimedeltaIndex")
raise TypeError("Cannot add non-tick DateOffset to {cls}"
.format(cls=type(self).__name__))

def _sub_offset_array(self, other):
# Array/Index of DateOffset objects
Expand All @@ -453,13 +459,19 @@ def _sub_offset_array(self, other):
else:
from pandas.errors import PerformanceWarning
warnings.warn("Adding/subtracting array of DateOffsets to "
"{} not vectorized".format(type(self)),
"{cls} not vectorized"
.format(cls=type(self).__name__),
PerformanceWarning)
res_values = self.astype('O').values - np.array(other)
return self.__class__(res_values, freq='infer')
except AttributeError:
raise TypeError("Cannot subtrack non-tick DateOffset from"
" TimedeltaIndex")
raise TypeError("Cannot subtrack non-tick DateOffset "
"from {cls}".format(cls=type(self).__name__))

@Substitution(klass='TimedeltaIndex')
@Appender(DatetimeIndexOpsMixin.shift.__doc__)
def shift(self, n, freq=None):
return super(TimedeltaIndex, self).shift(n, freq)

def _format_native_types(self, na_rep=u('NaT'),
date_format=None, **kwargs):
Expand Down Expand Up @@ -928,7 +940,7 @@ def insert(self, loc, item):

def delete(self, loc):
"""
Make a new DatetimeIndex with passed location(s) deleted.
Make a new TimedeltaIndex with passed location(s) deleted.

Parameters
----------
Expand Down