-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
[ENH] Use default EA repr for IntervalArray #26316
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report
@@ Coverage Diff @@
## master #26316 +/- ##
===========================================
- Coverage 92.04% 40.74% -51.31%
===========================================
Files 175 175
Lines 52291 52291
===========================================
- Hits 48132 21306 -26826
- Misses 4159 30985 +26826
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #26316 +/- ##
=========================================
Coverage ? 91.97%
=========================================
Files ? 180
Lines ? 50774
Branches ? 0
=========================================
Hits ? 46701
Misses ? 4073
Partials ? 0
Continue to review full report at Codecov.
|
I'm not sure we need / want to make this change. |
Also not sure if we need/want this change, as it adds an additional layer of complexity to maintaining common docstrings. Certainly something we could work around if we want to change the repr but not ideal. cc @jorisvandenbossche : what are your thoughts here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a some additional IntervalArray
docstrings that would need to be updated to match the new repr that aren't currently being checked by CI, e.g. from_arrays
, from_breaks
, etc.
pandas/pandas/core/arrays/interval.py
Lines 252 to 257 in bb0376c
Examples | |
-------- | |
>>> pd.%(qualname)s.from_breaks([0, 1, 2, 3]) | |
%(klass)s([(0, 1], (1, 2], (2, 3]], | |
closed='right', | |
dtype='interval[int64]') |
pandas/core/indexes/interval.py
Outdated
@@ -1086,8 +1086,52 @@ def equals(self, other): | |||
self.right.equals(other.right) and | |||
self.closed == other.closed) | |||
|
|||
@Appender(_interval_shared_docs['overlaps'] % _index_doc_kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there should be a way of reusing the main portion of the docstring, and appending in custom examples. Something similar to the constructor maybe?
pandas/pandas/core/indexes/interval.py
Lines 100 to 124 in bb0376c
@Appender(_interval_shared_docs['class'] % dict( | |
klass="IntervalIndex", | |
summary="Immutable index of intervals that are closed on the same side.", | |
name=_index_doc_kwargs['name'], | |
versionadded="0.20.0", | |
extra_attributes="is_overlapping\nvalues\n", | |
extra_methods="contains\n", | |
examples=textwrap.dedent("""\ | |
Examples | |
-------- | |
A new ``IntervalIndex`` is typically constructed using | |
:func:`interval_range`: | |
>>> pd.interval_range(start=0, end=5) | |
IntervalIndex([(0, 1], (1, 2], (2, 3], (3, 4], (4, 5]], | |
closed='right', | |
dtype='interval[int64]') | |
It may also be constructed using one of the constructor | |
methods: :meth:`IntervalIndex.from_arrays`, | |
:meth:`IntervalIndex.from_breaks`, and :meth:`IntervalIndex.from_tuples`. | |
See further examples in the doc strings of ``interval_range`` and the | |
mentioned constructor methods. | |
"""), |
Personally I think in principle it would be nice to start using the same repr style for all Array classes. |
@TomAugspurger what is your objection here? this de-special cases IntervalArray repr and makes it like all other Arrays, a good thing. |
Having to rewrite the docstrings to pass doctests.
… On May 11, 2019, at 14:16, Jeff Reback ***@***.***> wrote:
@TomAugspurger what is your objection here? this de-special cases IntervalArray repr and makes it like all other Arrays, a good thing.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
@makbigc can you merge master. |
working. Give me some more time. |
23322a6
to
c7121a7
Compare
@jschendel Amended. Please tell me if anything is wrong. |
ok this looks pretty good now; @jschendel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a whatsnew note with previous/current behavior examples to the 1.0 whatsnew since this is technically a backwards incompatible change?
Will review this in more depth later tonight but looks good overall. Would be nice to have an automated way of converting between |
doc/source/whatsnew/v1.0.0.rst
Outdated
|
||
.. ipython:: python | ||
|
||
In [2]: pd.arrays.IntervalArray.from_tuples([(0, 1), (2, 3)]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is an actual python block I don't think you need the In/Out - that may be causing the CI failure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WillAyd
Thanks for your advice. Otherwise I can't pass the CI tests.
@WillAyd @jschendel Anything else? |
doc/source/whatsnew/v1.0.0.rst
Outdated
|
||
*pandas 1.0.0* | ||
|
||
.. code-block:: ipython |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry could have been clearer but just make this `.. ipython:: python`` for the directive. You shouldn't then need the In / Out below and it will render automatically for you (this is typically what we do with new behavior)
doc/source/whatsnew/v1.0.0.rst
Outdated
|
||
*pandas 1.0.0* | ||
|
||
.. code-block:: python |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case it helps you should use code-block
for the old behavior and the ipython
directive for newly introduced behavior. You can see an example of this with the Better Repr for MultiIndex section back in the 0.25 whatsnew
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @makbigc! |
git diff upstream/master -u -- "*.py" | flake8 --diff