REF: simplify latex formatting #35872

ivanovmg · 2020-08-24T14:32:13Z

closes QST: need to refactor pandas/io/formats/latex.py? #35790
tests added pandas/tests/io/formats/test_latex.py
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff

Refactor to_latex using polymorphism and the builder design pattern.

Polymorphism

Previously there was a complicated logic in multiple methods
for either longtable or regular table.
This PR implements various builders:

RegularTableBuilder
LongTableBuilder
TabularBuilder

Selection of the appropriate builder is carried out in LatexFormatter,
depending on the kwargs provided.

Each builder must implement construction of all the table's components:

Beginning and end of the environment,
Separators (top, middle, bottom),
Header,
Body.

This allows one eliminate complex logic throughout multiple methods.

Separate Concerns

Separated concerns: row formatting into string (separate class RowStringConverter)
is carried out independently of the table builder.
Derived from RowStringConverter are two classes used to iterate over headers or body of the tables:

RowHeaderIterator
RowBodyIterator

Extract Methods

Multiple methods were extracted to improve code readability.

Previously there was a complicated logic in multiple methods for either longtable or regular table. This commit implements - ``LatexTableFormatter``, - ``LatexTabularFormatter``, - ``LatexLongTableFormatter``, derived from ``LatexFormatter``, based on ``LatexFormatterAbstract``. Each of the derived classes implements its own methods for writing - beginning of the table; - caption and labels; - separators; - end of the table. LatexFormatter changes ---------------------- - Make the process of creating tables more readable. - Drop escape and bold_rows attr

New classes for iterations over headers and body: - RowHeaderIterator; - RowBodyIterator, based on RowCreator.

This reverts ``pandas/io/formats/format.py`` to its nearly original state and enables polymorphism under the hood inside ``pandas/io/formats/latex.py``. Add ``pandas/tests/io/formats/test_latex.py`` to test lower-level functions/classes declared in ``pandas/io/formats/latex.py``

arw2019

thanks for the PR!

According to the code coverage bot under these changes patches of code are not covered by the tests. The thing to do there is for each patch either clean it up or add tests so that they're hit

Also, if I've got this right you're adding/removing methods. For new methods we'll need docstrings; old methods need to be deprecated prior to removal

ivanovmg · 2020-08-25T21:25:52Z

What is not covered by the tests are the abstract methods.

    @property
    @abstractmethod
    def env_end(self):
        pass

I think that the best thing is to setup coverage in such a way to ignore simple pass in the function.
Alternatively I can simply put a docstring there, which will be considered by the bot as a check.

The methods removed (actually one, _write_tabular_begin) and renamed (_print_cline -> _compose_cline) are internal implementation. Do I need to keep them and mark deprecated? They are not used anywhere else.

Also, do you want me to add docstrings for the internal methods? Or only public ones?
I mean, it is probably not reasonable to add docstrings to _escape_symbols or _convert_to_bold.
Unless required.

pandas/io/formats/latex.py

pandas/tests/io/formats/test_latex.py

arw2019

Re: deprecation I'm not sure. I would think it's not necessary as long as the methods are not exposed to users.

I would maybe change the title of the PR to something like "REF: simplify latex formatting" to make it clearer to other reviews what your goal is.

Otherwise looks okay to me. You'll need feedback from the core team to move forward

jreback

generally this is easier to follow. It feels slightly java-esque from the naming and builder pattern., but not too overboard. ok on the inheritance; almost always prefer composition, but this is not crazy. some comments.

pandas/io/formats/latex.py

jreback · 2020-08-27T02:25:31Z

pandas/io/formats/latex.py

+        return self._nlevels if self.fmt.header else 0
+
+    @property
+    def _ilevels(self):


i would make more descriptive properties here; they don't really need to be private, but ok

e.g.

index_nlevels is more descriptive.

Actually, it was not clear for me what these _ilevels-like values were in the original code. So, I decided not to change the names. As you suggested, I renamed them.

Hopefully, this is reasonable:

_ilevels -> index_levels _clevels -> column_levels _nlevels -> header_levels -- that was difficult for me to understand what it means

jreback · 2020-08-27T02:26:07Z

pandas/io/formats/latex.py


-        buf.write("\\toprule\n")
+    @staticmethod
+    def _escape_symbols(row):


ideally type every parameter both in and out

pandas/io/formats/latex.py

jreback · 2020-08-27T02:28:57Z

pandas/io/formats/latex.py

-        buf : string or file handle
-            File path or object. If not specified, the result is returned as
-            a string.
+    @property


this should be a class method no?

Not necessarily. At least not according to this https://refactoring.guru/design-patterns/builder/python/example

jreback · 2020-08-27T02:29:11Z

pandas/io/formats/latex.py

+                self._get_index_format() + self._get_column_format_based_on_dtypes()
+            )
+        elif not isinstance(input_column_format, str):  # pragma: no cover
+            raise AssertionError(


is this hit in the tests?

This is the original implementation.
I have just replaced AssertionError with ValueError and added several a test on this.

Problems -------- Could not figure out how to deal with the following. 1. Property setters 2. Inability of mypy to figure out that list can get iterator as argument rather than iterable only. pandas/io/formats/latex.py:44: error: Argument 1 to "list" has incompatible type "Iterator[Tuple[Any, ...]]"; expected "Iterable[List[str]]" [arg-type] pandas/io/formats/latex.py:544: error: Incompatible types in assignment (expression has type "Optional[str]", variable has type "str") [assignment]

ivanovmg · 2020-09-04T16:13:20Z

@jreback, is there anything else to be done from my side? It seems that I covered all the points you raised previously.

jreback

a couple of comments, pls merge master and ping on green.

pandas/io/formats/latex.py

jreback · 2020-09-05T19:59:24Z

pandas/io/formats/latex.py

+            position=self.position,
+        )
+
+    def _select_builder(self) -> Type[TableBuilderAbstract]:


I would inline thisin the builder function

I had to extract this method to make mypy happy.

If I refactor to this (which was originally there).

@property def builder(self) -> TableBuilderAbstract: kwargs = dict( formatter=self.fmt, column_format=self.column_format, multicolumn=self.multicolumn, multicolumn_format=self.multicolumn_format, multirow=self.multirow, caption=self.caption, label=self.label, position=self.position, ) if self.longtable: return LongTableBuilder(**kwargs) if any([self.caption, self.label, self.position]): return RegularTableBuilder(**kwargs) return TabularBuilder(**kwargs)

then mypy complains with the following error.

pandas/io/formats/latex.py:578: error: Argument 1 to "LongTableBuilder" has incompatible type "**Dict[str, object]"; expected "DataFrameFormatter" [arg-type] pandas/io/formats/latex.py:578: error: Argument 1 to "LongTableBuilder" has incompatible type "**Dict[str, object]"; expected "Optional[str]" [arg-type] pandas/io/formats/latex.py:578: error: Argument 1 to "LongTableBuilder" has incompatible type "**Dict[str, object]"; expected "bool" [arg-type] pandas/io/formats/latex.py:580: error: Argument 1 to "RegularTableBuilder" has incompatible type "**Dict[str, object]"; expected "DataFrameFormatter" [arg-type] pandas/io/formats/latex.py:580: error: Argument 1 to "RegularTableBuilder" has incompatible type "**Dict[str, object]"; expected "Optional[str]" [arg-type] pandas/io/formats/latex.py:580: error: Argument 1 to "RegularTableBuilder" has incompatible type "**Dict[str, object]"; expected "bool" [arg-type] pandas/io/formats/latex.py:581: error: Argument 1 to "TabularBuilder" has incompatible type "**Dict[str, object]"; expected "DataFrameFormatter" [arg-type] pandas/io/formats/latex.py:581: error: Argument 1 to "TabularBuilder" has incompatible type "**Dict[str, object]"; expected "Optional[str]" [arg-type] pandas/io/formats/latex.py:581: error: Argument 1 to "TabularBuilder" has incompatible type "**Dict[str, object]"; expected "bool" [arg-type]

Honestly, typing stuff makes life more complicated, IMHO.

yeap typing does make things more complicated, no arg there :->

- TableBuilder -> GenericTableBuilder - Add doctests explaining what each table builder does

pep8speaks · 2020-09-06T17:04:05Z

Hello @ivanovmg! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-09-06 17:57:25 UTC

jreback

lgtm ping on green.

ivanovmg · 2020-09-07T12:56:18Z

Can anybody restart the task Windows py37_np16?
It is unlikely that the failure in TestPandasContainer.test_to_s3 is anyhow related to the present PR.

jreback · 2020-09-07T18:59:00Z

thanks @ivanovmg for this, very nice!

ivanovmg added 5 commits August 24, 2020 21:12

Move strcols from property to attr for performance

104f968

Separate row format from LatexFormatting

18bc3bf

New classes for iterations over headers and body: - RowHeaderIterator; - RowBodyIterator, based on RowCreator.

Use property for table components

83fae7d

ivanovmg changed the title ~~Refactor/to latex~~ REF: Implement polymorphism in latex formatting Aug 24, 2020

Fix linting

f601598

ivanovmg force-pushed the refactor/to_latex branch from cbe59db to f601598 Compare August 24, 2020 15:24

REF: simplify logic in _caption_and_label

3d5d658

arw2019 suggested changes Aug 25, 2020

View reviewed changes

Add docstrings

7e18adc

arw2019 reviewed Aug 25, 2020

View reviewed changes

pandas/io/formats/latex.py Outdated Show resolved Hide resolved

arw2019 reviewed Aug 25, 2020

View reviewed changes

pandas/tests/io/formats/test_latex.py Outdated Show resolved Hide resolved

ivanovmg added 2 commits August 26, 2020 04:40

Add type hints to RowStringConverter

d200c70

Move tests from test_latex to test_to_latex

47416cf

ivanovmg mentioned this pull request Aug 26, 2020

ENH: Enable short_caption in to_latex #35668

Merged

5 tasks

arw2019 approved these changes Aug 26, 2020

View reviewed changes

ivanovmg changed the title ~~REF: Implement polymorphism in latex formatting~~ REF: simplify latex formatting Aug 26, 2020

jreback added IO LaTeX to_latex Refactor Internal refactoring of code labels Aug 27, 2020

jreback requested changes Aug 27, 2020

View reviewed changes

ivanovmg added 8 commits August 27, 2020 12:11

Rename _ilevels, _nlevels, _clevels as reviewed

2aa05cc

Move static methods to module level funcs

b06846c

Add test for incorrect column_format

a753ed5

Remove unnecessary list call

c9dd27e

Drop comment

bbcd930

Set more descriptive _xlevels names

115ed84

Ignore two problematic type checks

8e3c729

ivanovmg requested a review from jreback August 27, 2020 09:08

jreback added this to the 1.2 milestone Sep 5, 2020

jreback requested changes Sep 5, 2020

View reviewed changes

ivanovmg added 4 commits September 6, 2020 23:33

REF: rename product property to get_result()

1892b15

REF: rename base builder, add doctests

3e6a829

- TableBuilder -> GenericTableBuilder - Add doctests explaining what each table builder does

Merge branch 'master' into refactor/to_latex

d1b2acb

TYP: ignore specific mypy errors

94d8dc7

FIX: long line with type ignore

c4da0a2

jreback approved these changes Sep 6, 2020

View reviewed changes

jreback merged commit 773f64d into pandas-dev:master Sep 7, 2020

ivanovmg mentioned this pull request Sep 8, 2020

QST: need to refactor pandas/io/formats/latex.py? #35790

Closed

jbrockmendel pushed a commit to jbrockmendel/pandas that referenced this pull request Sep 8, 2020

REF: simplify latex formatting (pandas-dev#35872)

4550cf1

kesmit13 pushed a commit to kesmit13/pandas that referenced this pull request Nov 2, 2020

REF: simplify latex formatting (pandas-dev#35872)

ac640db

ivanovmg deleted the refactor/to_latex branch November 6, 2020 15:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

REF: simplify latex formatting #35872

REF: simplify latex formatting #35872

ivanovmg commented Aug 24, 2020 •

edited

Loading

arw2019 left a comment

ivanovmg commented Aug 25, 2020 •

edited

Loading

arw2019 left a comment

jreback left a comment

jreback Aug 27, 2020

ivanovmg Aug 27, 2020 •

edited

Loading

jreback Aug 27, 2020

jreback Aug 27, 2020

ivanovmg Aug 27, 2020

jreback Aug 27, 2020

ivanovmg Aug 27, 2020

ivanovmg commented Sep 4, 2020

jreback left a comment

jreback Sep 5, 2020

ivanovmg Sep 6, 2020

jreback Sep 6, 2020

pep8speaks commented Sep 6, 2020 •

edited

Loading

jreback left a comment

ivanovmg commented Sep 7, 2020

jreback commented Sep 7, 2020

REF: simplify latex formatting #35872

REF: simplify latex formatting #35872

Conversation

ivanovmg commented Aug 24, 2020 • edited Loading

Polymorphism

Separate Concerns

Extract Methods

arw2019 left a comment

Choose a reason for hiding this comment

ivanovmg commented Aug 25, 2020 • edited Loading

arw2019 left a 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

ivanovmg Aug 27, 2020 • edited Loading

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

ivanovmg commented Sep 4, 2020

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

pep8speaks commented Sep 6, 2020 • edited Loading

Comment last updated at 2020-09-06 17:57:25 UTC

jreback left a comment

Choose a reason for hiding this comment

ivanovmg commented Sep 7, 2020

jreback commented Sep 7, 2020

ivanovmg commented Aug 24, 2020 •

edited

Loading

ivanovmg commented Aug 25, 2020 •

edited

Loading

ivanovmg Aug 27, 2020 •

edited

Loading

pep8speaks commented Sep 6, 2020 •

edited

Loading