ENH: Add dtype parameter to IntervalIndex constructors and deprecate from_intervals #19339

jschendel · 2018-01-22T03:53:42Z

closes IntervalIndex constructor doesn't use dtype parameter #19262
closes Deprecate IntervalIndex.from_intervals in favor of the IntervalIndex constructor #19263
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

Summary:

Added support for a dtype parameter to all IntervalIndex constructors
- Allows users to override the inferred dtype
Deprecated IntervalIndex.from_intervals
- Still added dtype parameter, since it's just a pass through to IntervalIndex
- Removed usage and references to IntervalIndex.from_intervals throughout the codebase
Split construction tests off into interval/test_construction.py
- Created a base class for common tests, and subclasses for each constructor
- Was previously written in a more flat style, where each constructor was explicitly called
- Expanded the tests to hit some previously untested behavior

jschendel · 2018-01-22T04:00:33Z

doc/source/api.rst

@@ -1617,7 +1617,6 @@ IntervalIndex Components
   IntervalIndex.from_arrays
   IntervalIndex.from_tuples
   IntervalIndex.from_breaks
-   IntervalIndex.from_intervals


Should this be removed from api.rst now, or should I wait until from_intervals is actually deleted from the codebase and not just deprecated?

yes this is fine

codecov · 2018-01-22T09:07:50Z

Codecov Report

Merging #19339 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #19339      +/-   ##
==========================================
- Coverage   91.61%    91.6%   -0.02%     
==========================================
  Files         150      150              
  Lines       48677    48697      +20     
==========================================
+ Hits        44595    44607      +12     
- Misses       4082     4090       +8

Flag	Coverage Δ
#multiple	`89.97% <100%> (-0.02%)`	⬇️
#single	`41.72% <26.31%> (-0.02%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/indexes/category.py	`97.51% <100%> (ø)`	⬆️
pandas/core/reshape/tile.py	`90.25% <100%> (ø)`	⬆️
pandas/core/indexes/interval.py	`92.4% <100%> (-0.02%)`	⬇️
pandas/core/indexes/base.py	`96.46% <100%> (ø)`	⬆️
pandas/util/testing.py	`83.83% <0%> (-0.62%)`	⬇️

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 7202635...da2eac2. Read the comment docs.

jreback

small comments. testing looks great!

jreback · 2018-01-22T11:24:09Z

doc/source/api.rst

@@ -1617,7 +1617,6 @@ IntervalIndex Components
   IntervalIndex.from_arrays
   IntervalIndex.from_tuples
   IntervalIndex.from_breaks
-   IntervalIndex.from_intervals


yes this is fine

jreback · 2018-01-22T11:24:47Z

pandas/core/indexes/base.py

@@ -200,7 +200,8 @@ def __new__(cls, data=None, dtype=None, copy=False, name=None,
        # interval
        if is_interval_dtype(data) or is_interval_dtype(dtype):
            from .interval import IntervalIndex
-            return IntervalIndex(data, dtype=dtype, name=name, copy=copy)
+            return IntervalIndex(data, dtype=dtype, name=name, copy=copy,
+                                 **kwargs)


do we actually need kwargs to be passed or is that for consistentcy with others?

It's needed to pass closed.

can you pass that directly?

Done by doing closed = kwargs.get('closed', None), then passing that into the constructor. Not sure if you meant doing it that way, or by adding closed=None to the Index constructor.

jreback · 2018-01-22T11:25:05Z

pandas/core/indexes/interval.py

@@ -151,6 +152,8 @@ class IntervalIndex(IntervalMixin, Index):
        Name to be stored in the index.
    copy : boolean, default False
        Copy the meta-data
+    dtype : dtype or None, default None
+        If None, dtype will be inferred


add a version added

jreback · 2018-01-22T11:26:35Z

pandas/core/indexes/interval.py

@@ -417,6 +427,8 @@ def from_breaks(cls, breaks, closed='right', name=None, copy=False):
            Name to be stored in the index.
        copy : boolean, default False
            copy the data
+        dtype : dtype or None, default None
+            If None, dtype will be inferred


vesionadded

jreback · 2018-01-22T11:26:42Z

pandas/core/indexes/interval.py

@@ -458,6 +469,8 @@ def from_arrays(cls, left, right, closed='right', name=None, copy=False):
            Name to be stored in the index.
        copy : boolean, default False
            copy the data
+        dtype : dtype or None, default None
+            If None, dtype will be inferred


jreback · 2018-01-22T11:27:05Z

pandas/core/indexes/interval.py

@@ -545,6 +559,8 @@ def from_tuples(cls, data, closed='right', name=None, copy=False):
            Name to be stored in the index.
        copy : boolean, default False
            by-default copy the data, this is compat only and ignored
+        dtype : dtype or None, default None
+            If None, dtype will be inferred


jreback · 2018-01-22T11:27:28Z

pandas/core/indexes/interval.py

+                    lhs, rhs = d
+                except ValueError:
+                    msg = ('IntervalIndex.from_tuples requires tuples of '
+                           'length 2, got {tpl}').format(tpl=d)


are we testing both of these paths?

Yes, in TestFromTuples.test_constructor_errors: testing tuples of length 1 and 3, as well as non-tuple input.

jreback · 2018-01-22T11:27:56Z

pandas/core/indexes/interval.py

+                except TypeError:
+                    msg = ('IntervalIndex.from_tuples received an invalid '
+                           'item, {tpl}').format(tpl=d)
+                    raise TypeError(msg)
            left.append(lhs)
            right.append(rhs)

        # TODO
        # if we have nulls and we previous had *only*
        # integer data, then we have changed the dtype


I think this comment is moot? (this was from me originally).....

Agreed and removed. Was wondering about that comment but previously left it as-is in case there was something I missed; guess not.

jreback

very minor comments. ping on green.

jreback · 2018-01-23T11:38:28Z

pandas/core/indexes/interval.py

        left = _ensure_index(left, copy=copy)
        right = _ensure_index(right, copy=copy)

+        if dtype is not None:


can you add a 1-liner here, noting that this must be an interval dtype

jreback · 2018-01-23T11:39:21Z

pandas/core/indexes/interval.py

@@ -571,15 +595,21 @@ def from_tuples(cls, data, closed='right', name=None, copy=False):
            if isna(d):
                lhs = rhs = np.nan
            else:
-                lhs, rhs = d
+                try:


can you add a 1-liner here showing the format of th expected input (list-of-tuples)

…from_intervals

jschendel · 2018-01-25T01:09:42Z

@jreback : green

jreback · 2018-01-25T01:29:10Z

nice @jschendel keep em coming!

jschendel commented Jan 22, 2018

View reviewed changes

jreback added Deprecate Functionality to remove in pandas Interval Interval data type labels Jan 22, 2018

jsexauer mentioned this pull request Jan 22, 2018

DEPR: Clean up list of deprecations from prior versions #6581

Closed

1 task

jreback requested changes Jan 22, 2018

View reviewed changes

jschendel force-pushed the ii-constructor-dtype branch 2 times, most recently from 7b56db9 to 213b8eb Compare January 23, 2018 02:12

jreback approved these changes Jan 23, 2018

View reviewed changes

jreback added this to the 0.23.0 milestone Jan 23, 2018

jschendel mentioned this pull request Jan 24, 2018

Deprecate IntervalIndex.from_intervals in favor of the IntervalIndex constructor #19263

Closed

jschendel added 3 commits January 24, 2018 00:35

ENH: Add dtype parameter to IntervalIndex constructors and deprecate …

921376a

…from_intervals

review edits

a41c72e

additional review edits

da2eac2

jschendel force-pushed the ii-constructor-dtype branch from 213b8eb to da2eac2 Compare January 24, 2018 08:01

jreback merged commit 250574e into pandas-dev:master Jan 25, 2018

jschendel deleted the ii-constructor-dtype branch January 25, 2018 01:31

jreback mentioned this pull request Aug 7, 2019

DEPR: deprecations log for removed issues #13777

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Add dtype parameter to IntervalIndex constructors and deprecate from_intervals #19339

ENH: Add dtype parameter to IntervalIndex constructors and deprecate from_intervals #19339

jschendel commented Jan 22, 2018

jschendel Jan 22, 2018 •

edited

Loading

jreback Jan 22, 2018

codecov bot commented Jan 22, 2018 •

edited

Loading

jreback left a comment

jreback Jan 22, 2018

jreback Jan 22, 2018

jschendel Jan 23, 2018

jreback Jan 23, 2018

jschendel Jan 24, 2018

jreback Jan 22, 2018

jschendel Jan 23, 2018

jreback Jan 22, 2018

jschendel Jan 23, 2018

jreback Jan 22, 2018

jschendel Jan 23, 2018

jreback Jan 22, 2018

jschendel Jan 23, 2018

jreback Jan 22, 2018

jschendel Jan 23, 2018

jreback Jan 22, 2018

jschendel Jan 23, 2018

jreback left a comment

jreback Jan 23, 2018

jschendel Jan 24, 2018

jreback Jan 23, 2018

jschendel Jan 24, 2018

jschendel commented Jan 25, 2018

jreback commented Jan 25, 2018

ENH: Add dtype parameter to IntervalIndex constructors and deprecate from_intervals #19339

ENH: Add dtype parameter to IntervalIndex constructors and deprecate from_intervals #19339

Conversation

jschendel commented Jan 22, 2018

jschendel Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 22, 2018 • edited Loading

Codecov Report

jreback 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback 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

Choose a reason for hiding this comment

jschendel commented Jan 25, 2018

jreback commented Jan 25, 2018

jschendel Jan 22, 2018 •

edited

Loading

codecov bot commented Jan 22, 2018 •

edited

Loading