BUG: Convert float freqstrs to ints at finer resolution #14378

jisantuc · 2016-10-08T16:32:21Z

closes date_range treats '.5s' as '5s' #8419
tests added / passed
passes git diff upstream/master | flake8 --diff
whatsnew entry

Passing '0.5min' as a frequency string should generate 30 second
intervals, rather than five minute intervals. By recursively increasing
resolution until one is found for which the frequency is an integer,
this commit ensures that that's the case for resolutions from days to
microsecond

jreback · 2016-10-08T18:00:01Z

pandas/tseries/tests/test_frequencies.py

+        Reso = frequencies.Resolution
+
+        self.assertEqual(Reso.bump_to_int(1.5, 'T'), (90, 'S'))
+        self.assertEqual(Reso.bump_to_int(62.4, 'T'), (3744, 'S'))


can u exercise some more fail cases
and systematically cover cases that should succeed

Sure, added the failure case for being unable to convert to an int in a less toy example than starting all the way at the bottom and two success cases that together cover the entire implemented resolution range, from days --> milliseconds and from hours --> microseconds

codecov-io · 2016-10-08T18:24:57Z

Current coverage is 85.31% (diff: 100%)

Merging #14378 into master will increase coverage by <.01%

@@             master     #14378   diff @@
==========================================
  Files           144        144          
  Lines         50957      50972    +15   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          43469      43485    +16   
+ Misses         7488       7487     -1   
  Partials          0          0

Powered by Codecov. Last update 86233e1...65a5163

sinhrks · 2016-10-08T23:27:22Z

pandas/tseries/frequencies.py

+        else:
+            start_reso = cls.get_reso_from_freq(freq)
+            if start_reso == 0:
+                raise ValueError(_CANNOT_CONVERT_TO_INT)


pls move error message here, because the message is used only once.

sinhrks · 2016-10-08T23:32:53Z

pandas/tseries/frequencies.py

+        'T': 'S',
+        'S': 'L',
+        'L': 'U',
+        'U': None


Pls add Nanosecond.

sinhrks · 2016-10-08T23:35:32Z

pandas/tseries/tests/test_frequencies.py

@@ -108,10 +113,6 @@ def test_to_offset_invalid(self):
        with tm.assertRaisesRegexp(ValueError, 'Invalid frequency: -2D:3H'):
            frequencies.to_offset('-2D:3H')

-        # ToDo: Must be fixed in #8419
-        with tm.assertRaisesRegexp(ValueError, 'Invalid frequency: .5S'):
-            frequencies.to_offset('.5S')


Can you change it to compare with 500 milisec.

Pls add some tests whether this works with combined offset, like .5H.5S.

And also some failure cases with invalid str, such as 1.5.0S, 1.S.

1.S is valid now, parsed as 1 second. Should it not be?

You're right. should be 1 sec like float("1.") is valid.

sinhrks · 2016-10-08T23:40:36Z

doc/source/whatsnew/v0.19.1.txt

@@ -30,3 +30,5 @@ Performance Improvements

 Bug Fixes
 ~~~~~~~~~
+
+- Bug in date range treating e.g. 0.5s as 5s (:issue:`8419`)


can you add occurrence condition, like

Bug in multiple offset aliases with decimal points is regarded as int (e.g. 0.5s as 5s).

sinhrks · 2016-10-08T23:43:59Z

pandas/tseries/frequencies.py

@@ -160,6 +181,34 @@ def get_reso_from_freq(cls, freq):
        """
        return cls.get_reso(cls.get_str_from_freq(freq))

+    @classmethod
+    def bump_to_int(cls, value, freq):


can the method name be clearler, likeget_stride_from_decimal

sinhrks · 2016-10-08T23:45:06Z

pandas/tseries/frequencies.py

+    @classmethod
+    def bump_to_int(cls, value, freq):
+        """
+        Increase resolution an integer value is available to create an


Convert freq with decimal stride to higher freq with integer stride.

jreback · 2016-10-09T17:22:54Z

pandas/tseries/frequencies.py

-T_RESO = 3
-H_RESO = 4
-D_RESO = 5
+NS_RESO = 0


so would be nice to clean this up. This is essentially an Enum, but that only exists in 3.5 (maybe we could add the backport of https://pypi.python.org/pypi/enum34 as an in-inline package), not sure this is worth it though.

In any event just put these in a list and enumerate if needed (and you can put these directly in the class I think)

Just learned they need to be available at the module level for some of the c imports, but inside the class they can live in a list, so I did that.

They're used in some of the @classmethods it turns out so need to stay for now

jorisvandenbossche

Can you rebase and update?

jorisvandenbossche · 2016-10-27T14:01:22Z

doc/source/whatsnew/v0.19.1.txt

@@ -30,3 +30,5 @@ Performance Improvements

 Bug Fixes
 ~~~~~~~~~
+
+- Bug in multiple offset aliases with decimal points regarded as ints (e.g. 0.5s as 5s) (:issue:`8419`)


Can you move this to v0.20.0 file and add it to the enhancements section? It was a bug that is was parsed incorrectly and did not error, but supporting fractional resolutions is an enhancement that can be listed in the enhancement section

jorisvandenbossche · 2016-10-27T14:01:43Z

pandas/tseries/frequencies.py

-    RESO_MIN = T_RESO
-    RESO_HR = H_RESO
-    RESO_DAY = D_RESO
+    RESOS = [RESO_NS, RESO_US, RESO_MS, RESO_SEC, RESO_MIN, RESO_HR, RESO_DAY]


is this used somewhere?

Nope, will destruct.

Passing `'0.5min'` as a frequency string should generate 30 second intervals, rather than five minute intervals. By recursively increasing resolution until one is found for which the frequency is an integer, this commit ensures that that's the case for resolutions from days to microseconds. Fixes pandas-dev#8419

jreback · 2016-12-06T21:32:13Z

can you rebase

…14378

jreback · 2016-12-14T13:49:55Z

pandas/tseries/tests/test_frequencies.py

@@ -379,6 +402,25 @@ def test_freq_to_reso(self):
            result = Reso.get_freq(Reso.get_str(Reso.get_reso_from_freq(freq)))
            self.assertEqual(freq, result)

+    def test_resolution_bumping(self):
+        Reso = frequencies.Resolution


can you add the issue number here as as comment

jreback · 2016-12-14T13:50:19Z

pandas/tseries/frequencies.py

+    @classmethod
+    def get_stride_from_decimal(cls, value, freq):
+        """
+        Convert freq with decimal stride into a higher freq with integer stride


can you add Parameters/Returns section to detail the args

jreback · 2016-12-14T13:50:43Z

just a little docs, otherwise lgtm.

jorisvandenbossche · 2016-12-14T16:08:33Z

Made the latest changes quickly myself. @jisantuc Thanks a lot for this nice contribution!

…4378) Passing `'0.5min'` as a frequency string should generate 30 second intervals, rather than five minute intervals. By recursively increasing resolution until one is found for which the frequency is an integer, this commit ensures that that's the case for resolutions from days to microseconds. Fixes pandas-dev#8419

jreback requested changes Oct 8, 2016

View reviewed changes

sinhrks reviewed Oct 8, 2016

View reviewed changes

sinhrks added Frequency DateOffsets Bug labels Oct 8, 2016

jreback changed the title ~~Convert float freqstrs to ints at finer resolution~~ BUG: Convert float freqstrs to ints at finer resolution Oct 9, 2016

jreback reviewed Oct 9, 2016

View reviewed changes

jisantuc force-pushed the frequencies-float-to-int branch from 4ff2457 to 33664a6 Compare October 11, 2016 01:47

jorisvandenbossche reviewed Oct 27, 2016

View reviewed changes

jisantuc force-pushed the frequencies-float-to-int branch from 33664a6 to 47723c3 Compare October 27, 2016 14:29

Bring back resolution definitions and move whatnew to 0.20

01afe8a

jisantuc force-pushed the frequencies-float-to-int branch from fb422d3 to 01afe8a Compare October 27, 2016 15:05

jorisvandenbossche added 2 commits December 14, 2016 14:36

Merge remote-tracking branch 'upstream/master' into PR_TOOL_MERGE_PR_…

fd95822

…14378

reword whatsnew

65a5163

jreback reviewed Dec 14, 2016

View reviewed changes

latest comments

80b6d5a

jorisvandenbossche added this to the 0.20.0 milestone Dec 14, 2016

jorisvandenbossche merged commit 8b89ece into pandas-dev:master Dec 14, 2016

jisantuc deleted the frequencies-float-to-int branch January 18, 2019 18:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Convert float freqstrs to ints at finer resolution #14378

BUG: Convert float freqstrs to ints at finer resolution #14378

jisantuc commented Oct 8, 2016 •

edited

Loading

jreback Oct 8, 2016

jisantuc Oct 8, 2016

codecov-io commented Oct 8, 2016 •

edited

Loading

sinhrks Oct 8, 2016

sinhrks Oct 8, 2016

sinhrks Oct 8, 2016

sinhrks Oct 8, 2016 •

edited

Loading

jisantuc Oct 9, 2016

sinhrks Oct 11, 2016

sinhrks Oct 8, 2016

sinhrks Oct 8, 2016

sinhrks Oct 8, 2016

jreback Oct 9, 2016 •

edited

Loading

jisantuc Oct 11, 2016

jisantuc Oct 27, 2016

jorisvandenbossche left a comment

jorisvandenbossche Oct 27, 2016

jorisvandenbossche Oct 27, 2016

jisantuc Oct 27, 2016

jreback commented Dec 6, 2016

jreback Dec 14, 2016

jreback Dec 14, 2016

jreback Dec 14, 2016

jreback commented Dec 14, 2016

jorisvandenbossche commented Dec 14, 2016

BUG: Convert float freqstrs to ints at finer resolution #14378

BUG: Convert float freqstrs to ints at finer resolution #14378

Conversation

jisantuc commented Oct 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 8, 2016 • edited Loading

Current coverage is 85.31% (diff: 100%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sinhrks Oct 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback Oct 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Dec 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Dec 14, 2016

jorisvandenbossche commented Dec 14, 2016

jisantuc commented Oct 8, 2016 •

edited

Loading

codecov-io commented Oct 8, 2016 •

edited

Loading

sinhrks Oct 8, 2016 •

edited

Loading

jreback Oct 9, 2016 •

edited

Loading