ENH: between_time, at_time accept axis parameter #21799

yrhooke · 2018-07-07T20:13:04Z

xref #2141

closes ENH: add axis parameter to between_time, at_time #8839
tests added / passed

Axis parameter added to between_time and at_time, defaults to index for DataFrame

pep8speaks · 2018-07-07T20:13:08Z

Hello @yrhooke! Thanks for updating the PR.

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

Comment last updated on August 09, 2018 at 13:30 Hours UTC

codecov · 2018-07-08T02:46:12Z

Codecov Report

Merging #21799 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #21799      +/-   ##
==========================================
+ Coverage   92.24%   92.24%   +<.01%     
==========================================
  Files         161      161              
  Lines       51433    51441       +8     
==========================================
+ Hits        47446    47454       +8     
  Misses       3987     3987

Flag	Coverage Δ
#multiple	`90.64% <100%> (ø)`	⬆️
#single	`42.28% <7.69%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/generic.py	`96.84% <100%> (+0.01%)`	⬆️

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 91d1c50...9ae360f. Read the comment docs.

jreback

can you add a whatsnew in other enhancements for 0.24.0

jreback · 2018-07-08T13:07:10Z

pandas/core/generic.py

        Parameters
        ----------
        time : datetime.time or string
+        axis : int or string axis name, optional


can you add a versionadded 0.24.0 here (and other new kwargs)

jreback · 2018-07-08T13:07:26Z

pandas/core/generic.py

        end_time : datetime.time or string
        include_start : boolean, default True
        include_end : boolean, default True
+        axis : int or string axis name, optional


jreback · 2018-07-08T13:08:04Z

pandas/tests/frame/test_timeseries.py

+        stime, etime = ('08:00:00', '09:00:00')
+        dimn = (len(rng), len(rng))
+        exp_len = 7
+        for time_index, time_col in product([True, False], [True, False]):


can you parametrize this test instead of using a loop

jreback · 2018-07-08T13:08:23Z

pandas/tests/frame/test_timeseries.py

        with pytest.raises(TypeError):  # index is not a DatetimeIndex
            df.at_time('00:00')

+    def test_at_time_axis(self):


you can parametrize if you want

jreback · 2018-07-08T13:09:04Z

pandas/tests/series/test_timeseries.py

            assert len(ts.between_time(*time_string)) == expected_length

+    def test_between_time_axis(self):
+        rng = date_range('1/1/2000', periods=100, freq='10min')


can you add the issue number as a comment (to all new tests)

jreback · 2018-07-08T13:10:58Z

a nice followup would be #2141 (separate PR after this one)

yrhooke · 2018-07-08T22:54:50Z

Alright, I'm on it.

jreback · 2018-07-14T14:59:39Z

doc/source/whatsnew/v0.24.0.txt

 - :meth:`Series.nlargest`, :meth:`Series.nsmallest`, :meth:`DataFrame.nlargest`, and :meth:`DataFrame.nsmallest` now accept the value ``"all"`` for the ``keep`` argument. This keeps all ties for the nth largest/smallest value (:issue:`16818`)
 - :class:`IntervalIndex` has gained the :meth:`~IntervalIndex.set_closed` method to change the existing ``closed`` value (:issue:`21670`)
 - :func:`~DataFrame.to_csv` and :func:`~DataFrame.to_json` now support ``compression='infer'`` to infer compression based on filename (:issue:`15008`)
+- :func:`between_time` and :func:`at_time` now support an axis parameter (:issue: `8839`)


can you use double backticks around axis

jreback · 2018-07-14T15:00:38Z

pandas/core/generic.py

        try:
-            indexer = self.index.indexer_at_time(time, asof=asof)
-            return self._take(indexer)
+            index = self._get_axis(axis)


can you move the _get_axis out of the try/except (and the _take)

jreback · 2018-07-14T15:00:47Z

pandas/core/generic.py

+
        try:
-            indexer = self.index.indexer_between_time(
+            index = self._get_axis(axis)


same comment as above

jreback · 2018-07-14T15:01:57Z

pandas/tests/frame/test_timeseries.py

            df.at_time('00:00')

+    @pytest.mark.parametrize('time_axis', [
+        (False, False), (True, False), (False, True), (True, True)])


hmm this is an odd way to parameterize, can you just use axis as the argument?

jreback · 2018-07-14T15:02:03Z

pandas/tests/frame/test_timeseries.py

        with pytest.raises(TypeError):  # index is not a DatetimeIndex
            df.between_time(start_time='00:00', end_time='12:00')

+    @pytest.mark.parametrize('time_axis', [


same this is very hard to read

you can make the error case a separate test, which makes both tests much simpler

yrhooke · 2018-07-15T02:19:26Z

I hope it's alright now.

test_between_time_axis and test_between_time_axis_raises are trying to account for the scenarios where neither axis is a DatetimeIndex or that both are. If those are extraneous I can simplify them.

Also, should these tests be combined into the main test_between_time, test_at_time? I'm not sure what's preferred here.

jreback

small comments. pls rebase and ping on green.

jreback · 2018-07-20T13:23:17Z

pandas/tests/frame/test_timeseries.py

        with pytest.raises(TypeError):  # index is not a DatetimeIndex
            df.at_time('00:00')

+    @pytest.mark.parametrize('axis', ['index', 'columns'])


test for 0, 1 as well

jreback · 2018-07-20T13:23:27Z

pandas/tests/frame/test_timeseries.py

        with pytest.raises(TypeError):  # index is not a DatetimeIndex
            df.between_time(start_time='00:00', end_time='12:00')

+    @pytest.mark.parametrize('axis', [


also test for 0, 1

the new axis fixture will handle this, can you use that

which new axis fixture?

rebase in master and axis is available

why is this not just using the axis fixture?

I couldn't figure out what the axis fixture is. Is there documentation about this?

Here's the location of it. It just supplies the various arguments that are typically valid for an axis parameter

pandas/pandas/conftest.py

Line 65 in c04b773

def axis(request):

jreback · 2018-07-25T10:17:29Z

can you rebase and add those tests, otherwise lgtm.

jreback · 2018-08-02T10:48:43Z

can you rebase and update

yrhooke · 2018-08-03T12:12:34Z

I rebased and updated the tests, they're kind of awkwardly built, I couldn't figure out what the axis fixture was.

jreback · 2018-08-10T10:30:17Z

doc/source/whatsnew/v0.24.0.txt

 - :func:`to_timedelta` now supports iso-formated timedelta strings (:issue:`21877`)
 - :class:`Series` and :class:`DataFrame` now support :class:`Iterable` in constructor (:issue:`2193`)
 - :class:`DatetimeIndex` gained :attr:`DatetimeIndex.timetz` attribute. Returns local time with timezone information. (:issue:`21358`)
+- :func:`~DataFrame.to_csv` and :func:`~DataFrame.to_json` now support ``compression='infer'`` to infer compression based on filename (:issue:`15008`)


looks like you picked up something extra here

jreback · 2018-08-10T10:31:01Z

pandas/tests/frame/test_timeseries.py

        with pytest.raises(TypeError):  # index is not a DatetimeIndex
            df.at_time('00:00')

+    @pytest.mark.parametrize('axis', ['index', 'columns', 0, 1])


just remove the paramerize, the axis is already defined

jreback · 2018-08-10T10:31:50Z

pandas/tests/frame/test_timeseries.py

        with pytest.raises(TypeError):  # index is not a DatetimeIndex
            df.between_time(start_time='00:00', end_time='12:00')

+    @pytest.mark.parametrize('axis', [


why is this not just using the axis fixture?

jreback · 2018-08-10T10:32:01Z

pandas/tests/frame/test_timeseries.py

+            assert len(selected) == exp_len
+
+    @pytest.mark.parametrize('axis', [
+        (), ('index',), ('columns',), ('index', 'columns'),


same, this is very werid

jreback · 2018-09-23T21:42:49Z

think this got lost, can you rebase

jreback · 2018-10-18T16:19:08Z

can you merge master

jreback · 2018-11-18T18:49:22Z

ok i rebased this.

jreback · 2018-11-19T01:21:36Z

thanks @yrhooke

jreback added Enhancement Datetime Datetime data dtype labels Jul 7, 2018

jreback requested changes Jul 8, 2018

View reviewed changes

jreback requested changes Jul 14, 2018

View reviewed changes

jreback requested changes Jul 20, 2018

View reviewed changes

jreback added this to the 0.24.0 milestone Jul 20, 2018

yrhooke force-pushed the between-time-axis branch from 3a35e7b to f637b24 Compare August 3, 2018 11:59

yrhooke added 8 commits August 9, 2018 16:26

ENH: between_time, at_time accept axis parameter

b139cb1

PEP8 adjustment for ttests

51b7e61

added version/issue info for 8839

adf543d

whatsnew merge with master

bc83f9d

test_between_time parametrization simplified, raises test created

ad41871

0, 1 axes added to tests

5ea87f4

test_between_time_axis uses tuples

4ab35ee

flake8 fixes

35b4b41

yrhooke force-pushed the between-time-axis branch from dc9cf4f to 35b4b41 Compare August 9, 2018 13:30

jreback requested changes Aug 10, 2018

View reviewed changes

Merge branch 'master' into PR_TOOL_MERGE_PR_21799

2172bc9

use axis fixture

3a9550e

parameterize

9ae360f

jreback approved these changes Nov 19, 2018

View reviewed changes

jreback merged commit 97b612f into pandas-dev:master Nov 19, 2018

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

ENH: between_time, at_time accept axis parameter (pandas-dev#21799)

cd7fd0f

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

ENH: between_time, at_time accept axis parameter (pandas-dev#21799)

2cd11d5

Uh oh!

ENH: between_time, at_time accept axis parameter #21799

ENH: between_time, at_time accept axis parameter #21799

Uh oh!

Conversation

yrhooke commented Jul 7, 2018 • edited by jreback Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pep8speaks commented Jul 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated on August 09, 2018 at 13:30 Hours UTC

Uh oh!

codecov bot commented Jul 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jreback left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jreback commented Jul 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yrhooke commented Jul 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yrhooke commented Jul 15, 2018

Uh oh!

jreback left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jreback commented Jul 25, 2018

Uh oh!

jreback commented Aug 2, 2018

Uh oh!

yrhooke commented Aug 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

yrhooke commented Jul 7, 2018 •

edited by jreback

Loading

pep8speaks commented Jul 7, 2018 •

edited

Loading

codecov bot commented Jul 8, 2018 •

edited

Loading

jreback commented Jul 8, 2018 •

edited

Loading