REF: remove ExtensionArrayFormatter #26833

simonjayhawkins · 2019-06-13T17:42:06Z

precursor to_html formatter not called for float values in a mixed-type column (2) #26000
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

codecov · 2019-06-13T18:18:28Z

Codecov Report

Merging #26833 into master will increase coverage by 1.39%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #26833      +/-   ##
==========================================
+ Coverage   90.45%   91.85%   +1.39%     
==========================================
  Files         179      179              
  Lines       50706    50700       -6     
==========================================
+ Hits        45866    46570     +704     
+ Misses       4840     4130     -710

Flag	Coverage Δ
#multiple	`90.44% <100%> (-0.01%)`	⬇️
#single	`41.12% <20%> (?)`

Impacted Files	Coverage Δ
pandas/io/formats/format.py	`97.89% <100%> (-0.02%)`	⬇️
pandas/core/indexes/interval.py	`96.11% <100%> (ø)`	⬆️
pandas/core/arrays/base.py	`98.87% <0%> (-0.57%)`	⬇️
pandas/core/arrays/period.py	`98.3% <0%> (-0.25%)`	⬇️
pandas/core/arrays/sparse.py	`93.59% <0%> (-0.12%)`	⬇️
pandas/core/arrays/categorical.py	`95.8% <0%> (ø)`	⬆️
pandas/util/testing.py	`90.94% <0%> (+0.1%)`	⬆️
pandas/core/indexes/datetimes.py	`96.37% <0%> (+0.16%)`	⬆️
pandas/io/formats/printing.py	`86.09% <0%> (+1.6%)`	⬆️
pandas/io/clipboard/clipboards.py	`34.78% <0%> (+2.89%)`	⬆️
... and 4 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 e8132a2...a777fa4. Read the comment docs.

codecov · 2019-06-13T18:18:28Z

Codecov Report

Merging #26833 into master will increase coverage by 1.39%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #26833      +/-   ##
==========================================
+ Coverage   90.45%   91.85%   +1.39%     
==========================================
  Files         179      179              
  Lines       50706    50700       -6     
==========================================
+ Hits        45866    46570     +704     
+ Misses       4840     4130     -710

Flag	Coverage Δ
#multiple	`90.44% <100%> (-0.01%)`	⬇️
#single	`41.12% <20%> (?)`

Impacted Files	Coverage Δ
pandas/io/formats/format.py	`97.89% <100%> (-0.02%)`	⬇️
pandas/core/indexes/interval.py	`96.11% <100%> (ø)`	⬆️
pandas/core/arrays/base.py	`98.87% <0%> (-0.57%)`	⬇️
pandas/core/arrays/period.py	`98.3% <0%> (-0.25%)`	⬇️
pandas/core/arrays/sparse.py	`93.59% <0%> (-0.12%)`	⬇️
pandas/core/arrays/categorical.py	`95.8% <0%> (ø)`	⬆️
pandas/util/testing.py	`90.94% <0%> (+0.1%)`	⬆️
pandas/core/indexes/datetimes.py	`96.37% <0%> (+0.16%)`	⬆️
pandas/io/formats/printing.py	`86.09% <0%> (+1.6%)`	⬆️
pandas/io/clipboard/clipboards.py	`34.78% <0%> (+2.89%)`	⬆️
... and 4 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 e8132a2...a777fa4. Read the comment docs.

jreback · 2019-06-13T18:24:14Z

pandas/io/formats/format.py

+        if isinstance(values, (ABCIndexClass, ABCSeries)):
+            values = values._values
+
+        if is_categorical_dtype(values.dtype):


why would we not have both of these in the below if/elif clause?

we're deciding which values to use here in the same way as ExtensionArrayFormatter did.

the following if/else clause is selecting the Formatter class to use based on those values.

Seems like the idea is to extract the datetime64[ns] from the Categorical, and then reuse the Datetime64Formatter by going into the if / elif below.

yes i get the idea, trying to see if the logic can somehow be simpler

We could potentially define Categorical._formatter, which would provide the appropriate scalar formatter based on it's .dtype? Not sure if that'll work or not.

hmm we actually define this

def _formatter(self, boxed=False): # Defer to CategoricalFormatter's formatter. return None

maybe we are not really using this attribute fully?

format_array is used by to_string, to_html, _repr_html, to_latex and for the repr of many objects.

it should in theory be simpler and more generic.

it may be beneficial to move some logic out into the objects themselves so that format_array can work with any extension array and not require this special casing. (i think that is outside of the scope of this PR)

This PR is intended to remove the call to format_array from within ExtensionArrayFormatter so that the formatter parameter of format_array can be used for custom formatters wihout defaults being applied.

ExtensionArrayFormatter was dispatching back to format_array to then dispatch to the appropriate (another) Formatter.

maybe we are not really using this attribute fully?

agreed. many just return None to defer to the Formatters.

jreback · 2019-06-13T19:04:23Z

ok this is fine then. I agree, think need to refactor how format_array works, @simonjayhawkins maybe make an issue for it? to push more functionaily to the objects (and PandasArray) rather than encoding the conversions where they are.

simonjayhawkins · 2019-06-13T19:07:37Z

and we need to get rid of that leading_space business too?

jreback · 2019-06-13T19:21:20Z

and we need to get rid of that leading_space business too?

yes I think that's right

jorisvandenbossche · 2019-06-14T06:31:00Z

pandas/io/formats/format.py

-        if isinstance(values, (ABCIndexClass, ABCSeries)):
-            values = values._values
-
-        formatter = values._formatter(boxed=True)


@simonjayhawkins Is this still used in another place? (not very familiar with the formatting code, but wondering where this is called to ensure the underlying can determine the formatting)

I don't see it used elsewhere in this PR, or not anymore on master. It might be that we didn't have good tests to cover behaviour where the ExtensionArray deviated from the "normal" behaviour to catch this.

It might be that we didn't have good tests to cover behaviour where the ExtensionArray deviated from the "normal" behaviour to catch this.

i suspect that more tests will need to be added. particularly with cases where na_rep is passed to to_html etc or

we do have a specific issue here concerning EAs #25099

categorical, sparse, period etc return None (to defer) or just str (with boxed=True), so the _formatter was not adding anything.

for datetimelike we have

def _formatter(self, boxed=False): # TODO: Remove Datetime & DatetimeTZ formatters. return "'{}'".format

so will be removed.

for integer array, we have

def _formatter(self, boxed=False): def fmt(x): if isna(x): return 'NaN' return str(x) return fmt

so this is now not being called following the removal of the pre-formatting step. but this likely contributes to the na_rep issues.

IMO the _formatter methods should be used from within the Formatter classes, not passed to them. and cannot assign 'NaN' explcitily.

this should be part a subsequent refactor, see #26833 (comment) and #26837

i should have a follow-on ready shortly continuing the format_array cleanup. (maybe not till next week due to PyLondinium)

jorisvandenbossche · 2019-06-14T08:05:40Z

i should have a follow-on ready shortly continuing the format_array cleanup. (maybe not till next week due to PyLondinium)

I would then propose to revert this PR, and do a proper refactor all at once using this _formatter method in the Formatter class as you mention.
We might also need to think about if we need to add additonal arguments to _formatter to enable some of the options (but didn't look enough in detail to the formatting code to assess this)

The _formatter is essential to let EA authors override the formatting (and eg now master will break what I am working on in GeoPandas). It's a bit unfortunate we didn't have proper tests for this.

simonjayhawkins · 2019-06-14T08:26:38Z

The _formatter is essential to let EA authors override the formatting (and eg now master will break what I am working on in GeoPandas). It's a bit unfortunate we didn't have proper tests for this.

EA authors would therefore also need to ensure that additional formatting options can be honoured, eg max_colwidth, float_format, na_rep etc. Should this be their responsibility?

jorisvandenbossche · 2019-06-14T08:32:32Z

Well, that's indeed something to be discussed how to do that (that's what I meant above with looking into adding additional arguments to _formatter).

We probably should have done that thought exercise when introducing the _formatter method, but that's too late now and the _formatter is in the EA interface, so we should honor it (at least for external EAs).

simonjayhawkins · 2019-06-14T08:37:19Z

a formatters argument is in some of the to_* methods api, and we need to honour that too.

i'm OK if you want to revert for now though.

This reverts commit a00659a.

jorisvandenbossche · 2019-06-14T09:07:38Z

I added a revert in the PR where I am adding a test: #26845

jorisvandenbossche · 2019-06-14T09:08:16Z

Is there already an issue to have the general discussion of how to deal with formatting options and ExtensionArrays ?

simonjayhawkins · 2019-06-14T09:11:04Z

I added a revert in the PR where I am adding a test: #26845

no problem. i'll approve on green.

simonjayhawkins · 2019-06-14T09:13:06Z

Is there already an issue to have the general discussion of how to deal with formatting options and ExtensionArrays ?

i'll open a master tracker where we can add things like the Int64 na_rep issue.

…yFormatter removal (#26845) * TST: test custom _formatter for ExtensionArray * Revert "REF: remove ExtensionArrayFormatter (#26833)" This reverts commit a00659a.

ghost · 2019-06-17T12:48:14Z

i'll open a master tracker where we can add things like the Int64 na_rep issue.

@simonjayhawkins where is the discussion issue? I ran into another issues with formatters I'd like to discuss before moving forward with this there's a suggestion/feature request I'd like to put up.

simonjayhawkins · 2019-06-17T12:55:51Z

i'll open a master tracker where we can add things like the Int64 na_rep issue.

@simonjayhawkins where is the discussion issue? I ran into another issues with formatters I'd like to discuss before moving forward with this there's a suggestion/feature request I'd like to put up.

I've not opened a separate issue yet for three reasons

there may be overlap with Refactor format_array in pandas\io\formats\format.py #26837
awaiting the outcome of BUG: Fix series.round() handling of EA #26817
thinking about whether _formatting_values should have been deprecated.

feel free to open a new issue.

jorisvandenbossche · 2019-06-17T13:01:40Z

awaiting the outcome of #26817

How is the formatting discussion related to that PR?

jorisvandenbossche · 2019-06-17T13:02:41Z

We can maybe use #26837 as the general issue to discuss this?

simonjayhawkins · 2019-06-17T13:32:21Z

awaiting the outcome of #26817

How is the formatting discussion related to that PR?

IIUC correctly, before EAs, format_array formatted an numpy array. and GenericArrayFormatter expects an numpy array, used mainly for numpy arrays with dtype=object

but we now have a ExtensionArrayFormatter that includes logic to convert to a numpy array

        values = self.values
        if isinstance(values, (ABCIndexClass, ABCSeries)):
            values = values._values

        formatter = values._formatter(boxed=True)

        if is_categorical_dtype(values.dtype):
            # Categorical is special for now, so that we can preserve tzinfo
            array = values.get_values()
        else:
            array = np.asarray(values)

        fmt_values = format_array(array,

whereas before, _formatting_values was used and conversion to a numpy array looked like

    def _format_col(self, i):
        frame = self.tr_frame
        formatter = self._get_formatter(i)
        values_to_format = frame.iloc[:, i]._formatting_values()
        return format_array(values_to_format, formatter,
                            float_format=self.float_format, na_rep=self.na_rep,
                            space=self.col_space, decimal=self.decimal)

even though _formatting_values is depecated:

    def _formatting_values(self) -> np.ndarray:
        # At the moment, this has to be an array since we use result.dtype
        """
        An array of values to be printed in, e.g. the Series repr

        .. deprecated:: 0.24.0

           Use :meth:`ExtensionArray._formatter` instead.
        """

it is still used. but does not necessarily return an numpy array

>>> import pandas as pd
>>> df = pd.DataFrame({"c": [2,3,float('nan')]})
>>> df = df.astype("Int64")
>>> df.c._formatting_values()
<IntegerArray>
[2, 3, NaN]
Length: 3, dtype: Int64

so when creating a repr_html of a DataFrame which uses _format_col, we pass a <IntegerArray> to format_array, which then uses ExtensionArrayFormatter to get the values as an numpy array and then call
format_array again which then formats the numpy array using GenericArrayFormatter.

It seems a bit convoluted.

i'm not sure why a numpy array is not returned from _formatting_values.

I think the issues are related since the discussions revolve around numpy arrays from extension arrays and formatting probably works best if the formatting is done on a numpy array with am object dtype. performance is probably not an issue for formatting.

jorisvandenbossche · 2019-06-17T13:57:41Z

@simonjayhawkins That's exactly the discussion we should have. But not in a merged PR :-)
Can you just put that comment in a new issue? (or in #26837, same for me)

jorisvandenbossche · 2019-06-18T10:09:23Z

@pilkibun can you move your comment to #26837 as well?

ghost · 2019-06-19T00:34:25Z

@pilkibun can you move your comment to #26837 as well?

#26837 (comment)

REF: remove ExtensionArrayFormatter

a777fa4

simonjayhawkins added Output-Formatting __repr__ of pandas objects, to_string Refactor Internal refactoring of code labels Jun 13, 2019

jreback reviewed Jun 13, 2019

View reviewed changes

jreback added this to the 0.25.0 milestone Jun 13, 2019

jreback merged commit a00659a into pandas-dev:master Jun 13, 2019

simonjayhawkins mentioned this pull request Jun 13, 2019

Refactor format_array in pandas\io\formats\format.py #26837

Open

jorisvandenbossche reviewed Jun 14, 2019

View reviewed changes

jorisvandenbossche mentioned this pull request Jun 14, 2019

TST: test custom _formatter for ExtensionArray + revert ExtensionArrayFormatter removal #26845

Merged

jorisvandenbossche added a commit to jorisvandenbossche/pandas that referenced this pull request Jun 14, 2019

Revert "REF: remove ExtensionArrayFormatter (pandas-dev#26833)"

c982520

This reverts commit a00659a.

simonjayhawkins deleted the ExtensionArrayFormatter branch June 21, 2019 09:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

REF: remove ExtensionArrayFormatter #26833

REF: remove ExtensionArrayFormatter #26833

simonjayhawkins commented Jun 13, 2019

codecov bot commented Jun 13, 2019 •

edited

Loading

codecov bot commented Jun 13, 2019

jreback Jun 13, 2019

simonjayhawkins Jun 13, 2019

TomAugspurger Jun 13, 2019

jreback Jun 13, 2019

TomAugspurger Jun 13, 2019

jreback Jun 13, 2019

jreback Jun 13, 2019

simonjayhawkins Jun 13, 2019

jreback commented Jun 13, 2019

simonjayhawkins commented Jun 13, 2019

jreback commented Jun 13, 2019

jorisvandenbossche Jun 14, 2019

jorisvandenbossche Jun 14, 2019

simonjayhawkins Jun 14, 2019

jorisvandenbossche commented Jun 14, 2019

simonjayhawkins commented Jun 14, 2019

jorisvandenbossche commented Jun 14, 2019

simonjayhawkins commented Jun 14, 2019

jorisvandenbossche commented Jun 14, 2019

jorisvandenbossche commented Jun 14, 2019

simonjayhawkins commented Jun 14, 2019

simonjayhawkins commented Jun 14, 2019

ghost commented Jun 17, 2019

simonjayhawkins commented Jun 17, 2019

jorisvandenbossche commented Jun 17, 2019

jorisvandenbossche commented Jun 17, 2019

simonjayhawkins commented Jun 17, 2019 •

edited

Loading

jorisvandenbossche commented Jun 17, 2019

jorisvandenbossche commented Jun 18, 2019

ghost commented Jun 19, 2019

REF: remove ExtensionArrayFormatter #26833

REF: remove ExtensionArrayFormatter #26833

Conversation

simonjayhawkins commented Jun 13, 2019

codecov bot commented Jun 13, 2019 • edited Loading

Codecov Report

codecov bot commented Jun 13, 2019

Codecov Report

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 commented Jun 13, 2019

simonjayhawkins commented Jun 13, 2019

jreback commented Jun 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented Jun 14, 2019

simonjayhawkins commented Jun 14, 2019

jorisvandenbossche commented Jun 14, 2019

simonjayhawkins commented Jun 14, 2019

jorisvandenbossche commented Jun 14, 2019

jorisvandenbossche commented Jun 14, 2019

simonjayhawkins commented Jun 14, 2019

simonjayhawkins commented Jun 14, 2019

ghost commented Jun 17, 2019

simonjayhawkins commented Jun 17, 2019

jorisvandenbossche commented Jun 17, 2019

jorisvandenbossche commented Jun 17, 2019

simonjayhawkins commented Jun 17, 2019 • edited Loading

jorisvandenbossche commented Jun 17, 2019

jorisvandenbossche commented Jun 18, 2019

ghost commented Jun 19, 2019

codecov bot commented Jun 13, 2019 •

edited

Loading

simonjayhawkins commented Jun 17, 2019 •

edited

Loading