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

DOC: add Raises, Examples and See Also sections to methods at_time/between_time/first/last #20725

Merged
merged 3 commits into from May 7, 2018

Conversation

Projects
None yet
5 participants
@topper-123
Contributor

topper-123 commented Apr 17, 2018

Added note that methods at_time/between_time/first/last requires that the index is a DateTimeIndex.

Added functioning examples to the method doc strings.

Added See Also section to the mentioned method doc string.

EDIT: The doc strings for DatetimeIndex.indexer_at_time and DatetimeIndex.indexer_between_time said they return a timeseries, while in reality they return arrays. I've corrected that, and made some minor updates to those doc strings in addition.

@topper-123

This comment has been minimized.

Contributor

topper-123 commented Apr 17, 2018

The failures are in test_parquet and test_network, so unrelated to this PR.

@@ -6703,13 +6703,39 @@ def at_time(self, time, asof=False):
"""
Select values at particular time of day (e.g. 9:30AM).
Notes
-----
For this method to work, the index must to be a :class:`DateTimeIndex`

This comment has been minimized.

@jschendel

jschendel Apr 17, 2018

Member

The "T" should be lowercase in DatetimeIndex

This comment has been minimized.

@jschendel

jschendel Apr 17, 2018

Member

Likewise for the other "Notes" sections

--------
between_time : Select values between particular times of the day
first : select initial periods of time series based on a date offset
last: select final periods of time series based on a date offset

This comment has been minimized.

@jschendel

jschendel Apr 17, 2018

Member

The capitalization is inconsistent at the beginning of the descriptions: "S" vs "s" in select.

This comment has been minimized.

@jschendel

jschendel Apr 17, 2018

Member

Likewise in the other "See Also" sections

@codecov

This comment has been minimized.

codecov bot commented Apr 18, 2018

Codecov Report

Merging #20725 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #20725      +/-   ##
==========================================
+ Coverage   91.81%   91.82%   +0.01%     
==========================================
  Files         153      153              
  Lines       49481    49483       +2     
==========================================
+ Hits        45430    45437       +7     
+ Misses       4051     4046       -5
Flag Coverage Δ
#multiple 90.22% <100%> (+0.01%) ⬆️
#single 41.84% <0%> (-0.01%) ⬇️
Impacted Files Coverage Δ
pandas/core/indexes/datetimes.py 95.76% <ø> (ø) ⬆️
pandas/core/generic.py 96.12% <100%> (+0.14%) ⬆️
pandas/util/testing.py 84.59% <0%> (+0.2%) ⬆️

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 bd4332f...18025cf. Read the comment docs.

@topper-123

This comment has been minimized.

Contributor

topper-123 commented Apr 18, 2018

Comments from @jschendel have been addressed, thanks for the review.

@@ -6703,13 +6703,40 @@ def at_time(self, time, asof=False):
"""
Select values at particular time of day (e.g. 9:30AM).

This comment has been minimized.

@jreback

jreback Apr 21, 2018

Contributor

put this in a Raises section instead

2018-04-09 12:00:00 2
2018-04-10 00:00:00 3
2018-04-10 12:00:00 4
>>> ts.at_time('12:00')

This comment has been minimized.

@jreback

jreback Apr 21, 2018

Contributor

blank line between cases

By setting ``start_time`` to be later than ``end_time``,
you can get the times that are *not* between the two times.
Notes

This comment has been minimized.

@jreback

jreback Apr 21, 2018

Contributor

same as above

datetime.time or string in appropriate format ("%H:%M", "%H%M",
"%I:%M%p", "%I%M%p", "%H:%M:%S", "%H%M%S", "%I:%M:%S%p",
"%I%M%S%p").
If strings, then ``tseries.tools.to_time`` is used to convert to

This comment has been minimized.

@jreback

jreback Apr 21, 2018

Contributor

this last sentence is refering to an internal method, remove it

Returns
-------
values_at_time : TimeSeries
values_at_time : array

This comment has been minimized.

@jreback

jreback Apr 21, 2018

Contributor

boolean array

This comment has been minimized.

@topper-123

topper-123 Apr 21, 2018

Contributor

Actually it returns an array of integers (i.e. integer location)

Parameters
----------
start_time, end_time : datetime.time, str
datetime.time or string in appropriate format ("%H:%M", "%H%M",
"%I:%M%p", "%I%M%p", "%H:%M:%S", "%H%M%S", "%I:%M:%S%p",
"%I%M%S%p")
"%I%M%S%p").
If strings, then ``tseries.tools.to_time`` is used to convert to

This comment has been minimized.

@jreback

jreback Apr 21, 2018

Contributor

same

@jreback jreback added the Docs label Apr 21, 2018

@jreback

This comment has been minimized.

Contributor

jreback commented Apr 21, 2018

are these functions in api.rst?

@topper-123

This comment has been minimized.

Contributor

topper-123 commented Apr 21, 2018

are these functions in api.rst?

Yes for all of them.

@topper-123

This comment has been minimized.

Contributor

topper-123 commented Apr 21, 2018

Comments addessed.

@@ -6703,13 +6703,42 @@ def at_time(self, time, asof=False):
"""
Select values at particular time of day (e.g. 9:30AM).
Raises
-----
AttributeError

This comment has been minimized.

@jreback

jreback Apr 21, 2018

Contributor

really? this should be something else, prob a TypeError

This comment has been minimized.

@topper-123

topper-123 Apr 22, 2018

Contributor

Changed to TypeError, and whatsnew has gotten a note on this.

@@ -6985,17 +7054,46 @@ def first(self, offset):
Convenience method for subsetting initial periods of time series data
based on a date offset.
Raises
-----
NotImplementedError

This comment has been minimized.

@jreback

jreback Apr 21, 2018

Contributor

should also be a TypeError

This comment has been minimized.

@topper-123

topper-123 Apr 22, 2018

Contributor

Oh, I read the code wrong here, it already raised TypeError. doc string changed

@pep8speaks

This comment has been minimized.

pep8speaks commented Apr 22, 2018

Hello @topper-123! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on May 06, 2018 at 12:07 Hours UTC
@topper-123

This comment has been minimized.

Contributor

topper-123 commented Apr 22, 2018

Comments addressed.

@topper-123

This comment has been minimized.

Contributor

topper-123 commented Apr 25, 2018

Is this ok, @jreback?

"""
from pandas.tseries.frequencies import to_offset
if not isinstance(self.index, DatetimeIndex):
raise NotImplementedError("'first' only supports a DatetimeIndex "
"index")
raise TypeError("'first' only supports a DatetimeIndex index")

This comment has been minimized.

@jreback

jreback May 4, 2018

Contributor

can you add some tests that assert the TypeError? (was this not tested before)?

This comment has been minimized.

@topper-123

topper-123 May 5, 2018

Contributor

They were not there beforehand, so I've added.

A question: There exists test for Series.first/last, but not DataFrame.first/last. Should i copy the relevant tests from series/test_timeseries.py to frame/test_time_series.py, or is that not relevant?

This comment has been minimized.

@jreback

jreback May 5, 2018

Contributor

sure that would be great (do a search for them as well, it maybe that they are elsewhere, but they should be in test_timeseries

@jreback jreback added the Timeseries label May 4, 2018

@topper-123 topper-123 changed the title from DOC: add Notes, Examples and See Also to methods at_time/between_time/first/last to DOC: add Raises, Examples and See Also sections to methods at_time/between_time/first/last May 5, 2018

@topper-123

This comment has been minimized.

Contributor

topper-123 commented May 6, 2018

I've added the tests to frame/test_timeseries.py as discussed above and everything is green.

@jreback jreback added this to the 0.23.0 milestone May 7, 2018

@jreback

jreback approved these changes May 7, 2018

@jreback

This comment has been minimized.

Contributor

jreback commented May 7, 2018

if anyone has comment: @jorisvandenbossche @TomAugspurger @toobaz otherwise lgtm.

@TomAugspurger

This comment has been minimized.

Contributor

TomAugspurger commented May 7, 2018

@jreback jreback merged commit 587a0dd into pandas-dev:master May 7, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jreback

This comment has been minimized.

Contributor

jreback commented May 7, 2018

thank @topper-123

@topper-123 topper-123 deleted the topper-123:time_methods_docs branch May 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment