Skip to content

Fix arrow.Arrow.interval() - #466

Closed
dwrpayne wants to merge 3 commits into
arrow-py:masterfrom
dwrpayne:interval_fix
Closed

Fix arrow.Arrow.interval()#466
dwrpayne wants to merge 3 commits into
arrow-py:masterfrom
dwrpayne:interval_fix

Conversation

@dwrpayne

@dwrpayne dwrpayne commented Oct 9, 2017

Copy link
Copy Markdown

arrow.Arrow.interval() produces unexpected results if the interval doesn't evenly divide the range. It leaves off the last chunk of time.

$> start = datetime(2013, 5, 5, 12, 30)
$> end = datetime(2013, 5, 5, 17, 15)
$> for r in arrow.Arrow.interval('hour', start, end, 4): print (r)
(<Arrow [2013-05-05T12:00:00+00:00]>, <Arrow [2013-05-05T15:59:59.999999+00:00]>)

Expected result:

$> for r in arrow.Arrow.interval('hour', start, end, 4): print (r)
(<Arrow [2013-05-05T12:00:00+00:00]>, <Arrow [2013-05-05T15:59:59.999999+00:00]>)
(<Arrow [2013-05-05T16:00:00+00:00]>, <Arrow [2013-05-05T17:59:59.999999+00:00]>)

arrow.Arrow.interval() produces unexpected results if the interval doesn't evenly divide the range. It leaves off the last chunk of time.

>>> start = datetime(2013, 5, 5, 12, 30)
>>> end = datetime(2013, 5, 5, 17, 15)
>>> for r in arrow.Arrow.interval('hour', start, end, 4): print (r)
(<Arrow [2013-05-05T12:00:00+00:00]>, <Arrow [2013-05-05T15:59:59.999999+00:00]>)
# Expected result:
# (<Arrow [2013-05-05T12:00:00+00:00]>, <Arrow [2013-05-05T15:59:59.999999+00:00]>)
# (<Arrow [2013-05-05T16:00:00+00:00]>, <Arrow [2013-05-05T17:59:59.999999+00:00]>)
@andrewelkins

Copy link
Copy Markdown
Contributor

Can you add a test case for this?

@dwrpayne

dwrpayne commented Oct 11, 2017 via email

Copy link
Copy Markdown
Author

Test that interval() works correctly when the range isn't evenly divisible by the interval.
@codecov-io

codecov-io commented Oct 11, 2017

Copy link
Copy Markdown

Codecov Report

Merging #466 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #466   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          14     14           
  Lines        3189   3195    +6     
  Branches      229    230    +1     
=====================================
+ Hits         3189   3195    +6
Impacted Files Coverage Δ
arrow/arrow.py 100% <100%> (ø) ⬆️
tests/arrow_tests.py 100% <100%> (ø) ⬆️

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 b33925e...ad63085. Read the comment docs.

@richard-engineering

richard-engineering commented Nov 9, 2017

Copy link
Copy Markdown

Imho it would be better the last chunk of time be in a separate interval rather than appended to the last time's end date. That way you are guaranteed the maximum time span between any two dates is the specified interval rather than the last one potentially being higher. That seems to be how you implemented things (different from the original post)

@richard-engineering

richard-engineering commented Nov 9, 2017

Copy link
Copy Markdown

can also fix some pep8 issues along the way with this PR ;)

    if interval < 1:
        raise ValueError("interval has to be a positive integer")
    range_span = arrow.Arrow.span_range(frame, start, end, tz)
    bound = (len(range_span) // interval) * interval
    _range = [(range_span[i][0], range_span[i + interval - 1][1]) for i in range(0, bound, interval)]
    if bound < len(range_span):
        _range.append((range_span[bound][0], range_span[-1][1]))
    return _range

@dwrpayne

dwrpayne commented Nov 10, 2017

Copy link
Copy Markdown
Author

I'm confused. I wrote it so that if, for example, you ask for intervals of 2 hours over a 5 hour range, you are returned a list of length 3. The first two elements have a 2 hour interval and the 3rd has the extra hour.

The previous implementation would have dropped the 5th hour.

@dwrpayne

Copy link
Copy Markdown
Author

@rhan-mentad I think you misunderstood. I'm not appending the extra chunk of time to the last pair, I'm appending it to the return list.

Interval() returns a list of (start,end) tuples. I'm just appending a final (start,end) tuple in cases where there is a bit of time in the range left over.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants