BUG: Let IntervalIndex constructor override inferred closed #21584
Conversation
Codecov Report
@@ Coverage Diff @@
## master #21584 +/- ##
==========================================
+ Coverage 91.9% 91.9% +<.01%
==========================================
Files 153 153
Lines 49549 49546 -3
==========================================
- Hits 45537 45536 -1
+ Misses 4012 4010 -2
Continue to review full report at Codecov.
|
| @@ -317,13 +317,7 @@ def test_generic_errors(self, constructor): | |||
| pass | |||
|
|
|||
| def test_constructor_errors(self, constructor): | |||
| # mismatched closed inferred from intervals vs constructor. | |||
There was a problem hiding this comment.
hmm, I think this is an invalid interval as its closed on both sides?
There was a problem hiding this comment.
Yeah, my logic here is that if there's a conflict, then the user had to explicitly pass the conflicting value to the constructor since the default for closed is None. Being done explicitly seems to indicate (at least a degree of) user intention, so seems reasonable to defer to the user passed value if there's a conflict, as opposed to forcing preprocessing onto the user.
Can certainly see an argument for raising in this case though, so can modify to make the behavior consistent in regards to raising if that's the preferred behavior.
There was a problem hiding this comment.
so thinking of the scenario where you have a list of Intervals and you are then overriding to make it uniform, but this actually changes what this represents; the user doesn't even know this. maybe a warning would be in order? ISince the override is explicit the user 'knows what they are doing', but do they really?
There was a problem hiding this comment.
Hmm, not sure. I think it would be OK to override without warning or error, but not really sure.
There was a problem hiding this comment.
Not opposed to adding a warning; will do that within the next day. Shouldn't be much work and will be easy enough to remove if we decide we don't want it later.
Another use case for overriding is to change closed for an existing IntervalIndex. A little bit different than the list of intervals case, but I think we'd want the constructor to have consistent behavior for list of intervals input and IntervalIndex input.
On master the closed parameter is ignored for IntervalIndex input:
In [2]: index = pd.interval_range(0, 3, closed='both')
In [3]: pd.IntervalIndex(index, closed='neither')
Out[3]:
IntervalIndex([[0, 1], [1, 2], [2, 3]]
closed='both',
dtype='interval[int64]')And the best workaround (to my knowledge) is to deconstruct and pass to one of the from_* methods, which seems unnecessarily tedious, e.g.
In [4]: pd.IntervalIndex.from_arrays(index.left, index.right, closed='neither')
Out[4]:
IntervalIndex([(0, 1), (1, 2), (2, 3)]
closed='neither',
dtype='interval[int64]')
In [5]: pd.IntervalIndex.from_tuples(index.to_tuples(), closed='neither')
Out[5]:
IntervalIndex([(0, 1), (1, 2), (2, 3)]
closed='neither',
dtype='interval[int64]')There was a problem hiding this comment.
maybe think about adding a set_closed method (similar to how we have accessors for names/labels/levels and so on on Indexes), though in the construtor is fine as well.
ok so merge this as is and do followup? (either way ok)
There was a problem hiding this comment.
ok so merge this as is and do followup?
Merging as is. To be clear, for the followup are you referring to adding warnings, or just the set_closed?
git diff upstream/master -u -- "*.py" | flake8 --diffMakes
IntervalIndexconstructor behavior consistent: theclosedparameter, if specified, takes priority over the inferredclosed.Comments:
intervals_to_interval_boundsfunction to optionally raise if mixed values ofclosedare encountered instead of automatically raising.IntervalIndexfrom mixed closed lists, e.g.[Interval(0, 1, closed='left'), Interval(2, 3, closed='right')], only ifclosedis specified during construction.closedis not passed to the constructor.