ENH: GH17054: read_html() handles rowspan/colspan and infers headers #17089

jowens · 2017-07-26T23:43:35Z

closes ENH: read_html to handle rowspan, colspan #17054
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

jowens · 2017-07-27T16:15:23Z

@gfyoung can you help me reconcile failing tests in the CI checks vs. clean tests on my machine?

$ py.test-2.7 pandas/tests/io/test_html.py
============================= test session starts ==============================
platform darwin -- Python 2.7.13, pytest-3.1.3, py-1.4.34, pluggy-0.4.0
rootdir: /Users/jowens/Documents/src/pandas-jowens, inifile: setup.cfg
collected 76 items

pandas/tests/io/test_html.py ............................................................................

========================== 76 passed in 49.13 seconds ==========================

gfyoung · 2017-07-27T16:43:48Z

@jowens : Judging from your Travis build output, here's what I see:

You have a style-check error on the Python 2.7 machine. However, all tests are passing. That indicates your patch is working for Python 2.7. You should patch this first to get that build working at least.
Your changes are failing for Python 3.x, meaning that they are perhaps not compatible with Python 3. I would suggest that you create a separate environment using Python 3 as your interpreter (I see you are using Python 2), install your branch, and rerun tests to see if you can replicate the failures.

If you can, then continue developing in the Python 3.x environment to get tests passing. If you can't replicate, then that's going to be tricky to debug, but we can address that if we get there.

jowens · 2017-07-27T16:46:28Z

Can you point me to the "style-check error"?

gfyoung · 2017-07-27T16:49:31Z

The error can be found here. BTW, you can also see the error yourself by running the style-check command in the PR description:

git diff upstream/master -u -- "*.py" | flake8 --diff

jowens · 2017-07-27T16:50:29Z

Yeah, I did that before checking in:

$ git diff upstream/master -u -- "*.py" | flake8-2.7 --diff
$

Not sure what went wrong. I'm on it!

jowens · 2017-07-27T17:58:11Z

Cool! @gfyoung what's next?

jowens · 2017-08-29T15:14:04Z

@gfyoung any chance of moving this forward? I'd prefer not to have a private build of Pandas for the rest of my life.

gfyoung · 2017-08-29T15:37:33Z

@jowens : Sorry about that! My inbox is very full most of the time, so I think I just missed your comment about a month ago.

Let's do this:

Rebase onto master (and fix the merge conflict) to see if tests still pass
Ping @TomAugspurger for further review of the PR

jowens · 2017-08-29T15:58:24Z

@gfyoung FWIW the merge conflict is only "whatsnew"

jowens · 2017-08-29T16:00:21Z

@gfyoung because I really don't want to get this wrong and have a commit that's gonna be enormous, can you sketch out how I "rebase onto master" properly?

gfyoung · 2017-08-29T16:00:37Z

@jowens: True, but it's been a month since you've run the tests on Travis, Appveyor, etc. I just want to make sure that tests still pass.

gfyoung · 2017-08-29T16:02:28Z

@gfyoung because I really don't want to get this wrong and have a commit that's gonna be enormous, can you sketch out how I "rebase onto master" properly?

I would recommend to try using GitHub's UI editor for resolving the merge conflict. If that doesn't work out, I can give you further suggestions, but they will be all local (and probably) less visual.

gfyoung · 2017-08-29T16:02:46Z

Don't worry about rebasing onto master yet. Let's make sure this PR is merge-able first.

jowens · 2017-08-29T16:04:58Z

@gfyoung OK, looks like it's running and conflict-free. Stay tuned!

jowens · 2017-08-29T20:14:28Z

@TomAugspurger can you help me take the next step with this PR? Thanks @gfyoung!

TomAugspurger · 2017-08-30T12:44:14Z

I won't have a chance to review this in detail for a while, so if you think it's good then feel free to merge @gfyoung.

…

On Tue, Aug 29, 2017 at 3:14 PM, John Owens ***@***.***> wrote: @TomAugspurger <https://github.com/tomaugspurger> can you help me take the next step with this PR? Thanks @gfyoung <https://github.com/gfyoung>! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#17089 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQHIumSd6yu0bcha3VfFOCHko-9Fx8Aks5sdHErgaJpZM4Okn1p> .

jowens · 2017-09-06T16:18:48Z

@TomAugspurger @gfyoung could either one of you guys please take a look?

gfyoung · 2017-09-06T16:19:56Z

doc/source/whatsnew/v0.21.0.txt

 - Integration with `Apache Parquet <https://parquet.apache.org/>`__, including a new top-level :func:`read_parquet` and :func:`DataFrame.to_parquet` method, see :ref:`here <io.parquet>`.
 - :func:`DataFrame.add_prefix` and :func:`DataFrame.add_suffix` now accept strings containing the '%' character. (:issue:`17151`)
 - `read_*` methods can now infer compression from non-string paths, such as ``pathlib.Path`` objects (:issue:`17206`).
 - :func:`pd.read_sas()` now recognizes much more of the most frequently used date (datetime) formats in SAS7BDAT files (:issue:`15871`).
 - :func:`DataFrame.items` and :func:`Series.items` is now present in both Python 2 and 3 and is lazy in all cases (:issue:`13918`, :issue:`17213`)

-
-


Add these lines back.

Also, your whatsnew has a merge conflict, so you definitely need to fix that.

gfyoung · 2017-09-06T16:20:24Z

pandas/io/html.py


        Parameters
        ----------
-        rows : iterable of node-like
-            A list of row elements.
+        doc : tree-like


As tree-like is not a Python data-type, we should re-specify this.

@TomAugspurger : If you think "node-like" or "tree-like" is OK for this documentation, then we can ignore this comment (and ones similar to that). However, IMO we should have proper data types in the doc-strings if possible.

gfyoung · 2017-09-06T16:20:56Z

pandas/io/html.py

-    def _parse_td(self, obj):
-        """Return the td elements from a row element.
+    def _equals_tag(self, obj, tag):
+        """Returns whether an individual DOM node matches a tag

        Parameters
        ----------
        obj : node-like


Let's take advantage of this time to re-specify this as a proper data type if possible.

gfyoung · 2017-09-06T16:21:03Z

pandas/io/html.py


        Parameters
        ----------
        obj : node-like
+            A DOM node.
+
+        tag : string


string --> str

gfyoung · 2017-09-06T16:21:31Z

pandas/io/html.py

-        columns : list of node-like
-            These are the elements of each row, i.e., the columns.
+        boolean
+            Does the object match tag 'tag'?


Just say "boolean indicating the object matches the tag tag"

…ndas into read_html_with_colspan_rowspan

jowens · 2017-09-25T16:26:23Z

@TomAugspurger Next steps?

jowens · 2017-10-02T20:38:33Z

@TomAugspurger @gfyoung What can I do to move this forward?

gfyoung · 2017-10-02T20:51:10Z

@TomAugspurger : Can you take a look at this? I've already approved, and @jowens and I are awaiting your comments before proceeding.

TomAugspurger

Overall things look pretty good. I don't completely understand the logic in _expand_colspan_rowspan, and there were several lines that seem to be untested (perhaps they're dead code now?).

Could you attempt to refactor those lmaps to use list-comprehensions and chain? Could you add some more tests for

table with just a thead, but no body
table with just a tbody, but each row is a th

And cases where rowspan / colspan vary for each of those.

TomAugspurger · 2017-10-02T21:00:04Z

pandas/io/html.py

                           raise_with_traceback, binary_type)
 from pandas import Series
-from pandas.core.common import AbstractMethodError
+from pandas.core.common import (AbstractMethodError, flatten)


Don't need the parens

TomAugspurger · 2017-10-02T21:02:38Z

pandas/io/html.py

-            defined by subclasses.
+        match : str or regular expression
+            The text to search for in the DOM tree.
+


You can remove these blank lines between parameters

TomAugspurger · 2017-10-02T21:07:21Z

pandas/io/html.py

+
+        Returns
+        -------
+        tuple of (header, body, footer)


you can remove this line (multiple returns are always tuples)

TomAugspurger · 2017-10-02T21:08:19Z

pandas/io/html.py

-        footer = self._parse_raw_tfoot(table)
+    def _build_table(self, table_html):
+        header, body, footer = self._parse_raw_thead_tbody_tfoot(table_html)
+        # the above "footer" actually produces a footer. The below "footer"


What's the plan here? Do we have a GH issue open for replacing the old with the new? i.e., what does the person reading the code 3 years from now need to know whether this has been resolved?

TomAugspurger · 2017-10-02T21:09:29Z

pandas/io/html.py

+            while all(self._equals_tag(t, 'th') for t in
+                      self._extract_td(body_rows[-1])):
+                # this row should be a footer row, move it from body to footer
+                footer_rows.insert(0, body_rows.pop())


This seems to be untested.

TomAugspurger · 2017-10-02T21:13:45Z

pandas/io/html.py

+            # splitting criterion: if all tags within a row are th, it's part
+            # of the header/footer
+            while all(self._equals_tag(t, 'th') for t in
+                      self._extract_td(body_rows[0])):


What happens with a table of all "headers". Does the condition need to be while body_rows and all(...) (is it possible to get to this situation, and do you have a test for it)?

TomAugspurger · 2017-10-02T21:22:22Z

pandas/io/html.py

+                            for col in extracted_row]
+            # expand cols using col_colspans
+            # maybe this can be done with a list comprehension, dunno
+            cols = list(zip(


I think this is equivalent to

list(chain.from_iterable([text] * nc for text, nc in zip(cols_text, col_colspans)))

which I find a bit more readable. Can you confirm if that works? (import chain from itertools).

Hmm, reading a bit further, maybe not? But I think that's close. Could you post a sample of col_colspans, col_rowspans, and cols_texts? Are they all just flat lists of ints / strs?

TomAugspurger · 2017-10-02T21:28:11Z

pandas/io/html.py

+        return element.find_all('tr')
+
+    def _extract_th(self, element):
+        return element.find_all('th')


TomAugspurger · 2017-10-02T21:29:29Z

pandas/io/html.py

        return table.xpath('.//tfoot')

    def _parse_raw_thead(self, table):
        expr = './/thead'
        thead = table.xpath(expr)
        res = []
        if thead:
-            trs = self._parse_tr(thead[0])
+            trs = self._extract_tr(thead[0])


TomAugspurger · 2017-10-02T21:30:40Z

pandas/io/html.py

+    the header, otherwise the function attempts to find the header within
+    the body (by putting rows with only ``<th>`` elements into the header).
+
+        .. versionadded:: 0.21.0


This could be a versionmodified, though it may not be necessary.

jreback · 2017-11-12T19:04:33Z

needs to be rebased

jowens · 2017-11-12T19:07:26Z

@TomAugspurger made a lot of good suggestions that I simply haven't had time to get to yet. Is there an impending deadline here?

jreback · 2017-11-12T19:08:44Z

@jowens nope. just pinging on older pr's. rebase / fixup when you can.

ostrokach · 2017-12-05T15:58:39Z

@jowens I just tried out this branch for extracting tables from PubMed Central XML files, and and it works much much better than the current read_html implementation (which was basically unusable)! Thank you for your work guys! Hope this can make it into the master branch soon :).

jreback · 2018-02-10T18:42:26Z

can you git merge master resolve conflicts and push again (and move whatsnew to 0.23.0)

jowens · 2018-02-11T01:23:51Z

@TomAugspurger's suggestions are still all good ones, but I haven't had a full day where I can sit down and do them.

wkerzendorf · 2018-04-19T18:30:26Z

great work! can't wait for it to get merged

jowens · 2018-04-19T18:31:44Z

I'll repeat that "@TomAugspurger's suggestions are still all good ones, but I haven't had a full day where I can sit down and do them." :)

TomAugspurger · 2018-04-19T18:41:13Z

No rush, we're patient :)

…

On Thu, Apr 19, 2018 at 1:31 PM, John Owens ***@***.***> wrote: I'll repeat that ***@***.*** <https://github.com/TomAugspurger>'s suggestions are still all good ones, but I haven't had a full day where I can sit down and do them." :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#17089 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQHIl6-no2ugOPI4Jct6JpVcLw7hqRyks5tqNgVgaJpZM4Okn1p> .

cancan101 · 2018-06-13T04:21:14Z

any updates on this PR?

jowens · 2018-06-13T04:22:40Z

I'll repeat again that "@TomAugspurger's suggestions are still all good ones, but I haven't had a full day where I can sit down and do them." :)

adamhooper · 2018-06-13T18:30:29Z

@jowens Hi! This feature is at the top of my backlog this week. So effectively, I have a full day where I can sit down and incorporate @TomAugspurger's suggestions. Would that be okay with you?

jowens · 2018-06-13T18:32:17Z

Yes PLEASE! I would be delighted (and so would the other folks in this thread).

WillAyd · 2018-08-17T16:58:45Z

Superseded by #21487

ENH: GH17054: read_html() handles rowspan/colspan and infers headers

4bf2f2e

jowens mentioned this pull request Jul 26, 2017

Read html with colspan rowspan #17084

Closed

4 tasks

gfyoung added Enhancement IO HTML read_html, to_html, Styler.apply, Styler.applymap labels Jul 26, 2017

in python 3, lambdas no longer take tuples as args. thanks pep 3113.

80d9c2b

jowens added 2 commits July 27, 2017 09:52

fixing lint error

26d1f6a

in python3, zip does not return a list, so list(zip(...))

37af4ea

Merge branch 'master' into read_html_with_colspan_rowspan

86dee93

gfyoung reviewed Sep 6, 2017

View reviewed changes

pandas/io/html.py Outdated

Parameters

----------

obj : node-like

A DOM node.

tag : string

Copy link

Member

gfyoung Sep 6, 2017

Choose a reason for hiding this comment

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

string --> str

gfyoung reviewed Sep 6, 2017

View reviewed changes

jowens added 10 commits September 20, 2017 14:49

documentation changes only

91a2300

documentation changes only

41058ab

documentation changes only, limited to 80 cols

4926913

more documentation edits

14235ec

minor documentation edits

196c835

better return type explanation in code, added issue number to tests

fed4b03

cleaning up legacy documentation issues

c2d9cc6

remove 'if'

d4b213b

newlines for clarity

b16f6d5

Merge branch 'read_html_with_colspan_rowspan' of github.com:jowens/pa…

092889a

…ndas into read_html_with_colspan_rowspan

TomAugspurger reviewed Oct 2, 2017

View reviewed changes

adamhooper mentioned this pull request Jun 14, 2018

read_html: Handle colspan and rowspan #21487

Merged

5 tasks

WillAyd closed this Aug 17, 2018

ENH: GH17054: read_html() handles rowspan/colspan and infers headers #17089

ENH: GH17054: read_html() handles rowspan/colspan and infers headers #17089

Conversation

jowens commented Jul 26, 2017

jowens commented Jul 27, 2017

gfyoung commented Jul 27, 2017

jowens commented Jul 27, 2017

gfyoung commented Jul 27, 2017 • edited Loading

jowens commented Jul 27, 2017

jowens commented Jul 27, 2017

jowens commented Aug 29, 2017

gfyoung commented Aug 29, 2017

jowens commented Aug 29, 2017

jowens commented Aug 29, 2017

gfyoung commented Aug 29, 2017

gfyoung commented Aug 29, 2017

gfyoung commented Aug 29, 2017

jowens commented Aug 29, 2017

jowens commented Aug 29, 2017

TomAugspurger commented Aug 30, 2017 via email

jowens commented Sep 6, 2017

Choose a reason for hiding this comment

gfyoung Sep 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfyoung Sep 6, 2017 • 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

jowens commented Sep 25, 2017

jowens commented Oct 2, 2017

gfyoung commented Oct 2, 2017

TomAugspurger 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

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 Nov 12, 2017

jowens commented Nov 12, 2017

jreback commented Nov 12, 2017

ostrokach commented Dec 5, 2017

jreback commented Feb 10, 2018

jowens commented Feb 11, 2018

wkerzendorf commented Apr 19, 2018

jowens commented Apr 19, 2018

TomAugspurger commented Apr 19, 2018 via email

cancan101 commented Jun 13, 2018

jowens commented Jun 13, 2018

adamhooper commented Jun 13, 2018

jowens commented Jun 13, 2018

WillAyd commented Aug 17, 2018

gfyoung commented Jul 27, 2017 •

edited

Loading

gfyoung Sep 6, 2017 •

edited

Loading

gfyoung Sep 6, 2017 •

edited

Loading