Skip to content
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

[ENH] Use default EA repr for IntervalArray #26316

Merged
merged 23 commits into from Sep 10, 2019

Conversation

makbigc
Copy link
Contributor

@makbigc makbigc commented May 8, 2019

@codecov
Copy link

codecov bot commented May 8, 2019

Codecov Report

Merging #26316 into master will decrease coverage by 51.3%.
The diff coverage is n/a.

Impacted file tree graph

@@             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
Flag Coverage Δ
#multiple ?
#single 40.74% <ø> (-0.15%) ⬇️
Impacted Files Coverage Δ
pandas/core/arrays/interval.py 38.15% <ø> (-54.92%) ⬇️
pandas/core/indexes/interval.py 35.33% <ø> (-59.9%) ⬇️
pandas/io/formats/latex.py 0% <0%> (-100%) ⬇️
pandas/io/sas/sas_constants.py 0% <0%> (-100%) ⬇️
pandas/core/groupby/categorical.py 0% <0%> (-100%) ⬇️
pandas/tseries/plotting.py 0% <0%> (-100%) ⬇️
pandas/tseries/converter.py 0% <0%> (-100%) ⬇️
pandas/io/formats/html.py 0% <0%> (-99.37%) ⬇️
pandas/io/sas/sas7bdat.py 0% <0%> (-91.16%) ⬇️
pandas/io/sas/sas_xport.py 0% <0%> (-90.1%) ⬇️
... and 132 more

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 23b6115...95c012a. Read the comment docs.

@codecov
Copy link

codecov bot commented May 8, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@be4b48e). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #26316   +/-   ##
=========================================
  Coverage          ?   91.97%           
=========================================
  Files             ?      180           
  Lines             ?    50774           
  Branches          ?        0           
=========================================
  Hits              ?    46701           
  Misses            ?     4073           
  Partials          ?        0
Flag Coverage Δ
#multiple 90.62% <100%> (?)
#single 41.83% <100%> (?)
Impacted Files Coverage Δ
pandas/core/arrays/interval.py 92.19% <100%> (ø)
pandas/core/indexes/interval.py 95.95% <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 be4b48e...645e01e. Read the comment docs.

@TomAugspurger
Copy link
Contributor

I'm not sure we need / want to make this change.

@gfyoung gfyoung added ExtensionArray Extending pandas with custom dtypes or arrays. Interval Interval data type labels May 8, 2019
@jschendel
Copy link
Member

jschendel commented May 10, 2019

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?

Copy link
Member

@jschendel jschendel left a 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.

Examples
--------
>>> pd.%(qualname)s.from_breaks([0, 1, 2, 3])
%(klass)s([(0, 1], (1, 2], (2, 3]],
closed='right',
dtype='interval[int64]')

@@ -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)
Copy link
Member

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?

@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.
"""),

@jorisvandenbossche
Copy link
Member

Personally I think in principle it would be nice to start using the same repr style for all Array classes.
But the docstring sharing is indeed something we first need to think through.
How do we currently do that for DatetimeArray? We don't share docstrings with DatetimeIndex?

@jreback
Copy link
Contributor

jreback commented May 11, 2019

@TomAugspurger what is your objection here? this de-special cases IntervalArray repr and makes it like all other Arrays, a good thing.

@TomAugspurger
Copy link
Contributor

TomAugspurger commented May 11, 2019 via email

@jreback
Copy link
Contributor

jreback commented Jun 8, 2019

@makbigc can you merge master.

@makbigc
Copy link
Contributor Author

makbigc commented Jun 12, 2019

working. Give me some more time.

@makbigc makbigc force-pushed the ENH-25022 branch 2 times, most recently from 23322a6 to c7121a7 Compare June 22, 2019 09:48
@makbigc
Copy link
Contributor Author

makbigc commented Jul 19, 2019

@​jschendel Amended. Please tell me if anything is wrong.

@jreback jreback added this to the 1.0 milestone Jul 21, 2019
@jreback
Copy link
Contributor

jreback commented Jul 21, 2019

ok this looks pretty good now; @jschendel

Copy link
Member

@jschendel jschendel left a 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?

@jschendel
Copy link
Member

Will review this in more depth later tonight but looks good overall. Would be nice to have an automated way of converting between IntervalArray and IntervalIndex example sections but that can be saved for a follow up.


.. ipython:: python

In [2]: pd.arrays.IntervalArray.from_tuples([(0, 1), (2, 3)])
Copy link
Member

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

Copy link
Contributor Author

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.

pandas/tests/arrays/interval/test_interval.py Show resolved Hide resolved
@makbigc
Copy link
Contributor Author

makbigc commented Sep 4, 2019

@WillAyd @jschendel Anything else?


*pandas 1.0.0*

.. code-block:: ipython
Copy link
Member

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)


*pandas 1.0.0*

.. code-block:: python
Copy link
Member

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

https://github.com/pandas-dev/pandas/blob/5f31bca3813b2ed71363a9d40518f217d2b546d6/doc/source/whatsnew/v0.25.0.rst#better-repr-for-multiindex

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jschendel jschendel merged commit 96bf661 into pandas-dev:master Sep 10, 2019
@jschendel
Copy link
Member

thanks @makbigc!

@makbigc makbigc deleted the ENH-25022 branch September 10, 2019 04:14
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ExtensionArray Extending pandas with custom dtypes or arrays. Interval Interval data type
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use default EA repr for IntervalArray
7 participants