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

Support N>1 spanners #345

Merged
merged 6 commits into from
May 15, 2024
Merged

Support N>1 spanners #345

merged 6 commits into from
May 15, 2024

Conversation

timkpaine
Copy link
Contributor

@timkpaine timkpaine commented May 14, 2024

Summary

Support more than 1 level of spanners:

from great_tables import GT, exibble
(
    GT(exibble)
    .tab_spanner("A", ["num", "char", "fctr"])
    .tab_spanner("B", ["fctr"])
    .tab_spanner("C", ["num", "char"])
    .tab_spanner("D", ["fctr", "date", "time"])
    .tab_spanner("E", spanners=["B", "C"])
)
Screenshot 2024-05-14 at 19 48 32

NOTE:

  • I made a design tweak with the underscore under upper level spanners
  • Needs tests
  • fix hack columns work that breaks columns=cs.all()
  • ensure stubhead is accounted for in col spans count

Related GitHub Issues and PRs

Checklist

Fixes: #273
Fixes: #221

@machow
Copy link
Collaborator

machow commented May 14, 2024

Thanks so much--this is really great! Did you want to work on the changes in note? We are more than happy to pick up and work on testing, if you're focused on Loc stuff. This is super helpful--happy to do whatever is most useful!

@timkpaine
Copy link
Contributor Author

# TODO
from great_tables import GT, exibble

(
    GT(exibble, rowname_col="row", groupname_col="group")
    .tab_spanner("A", ["num", "char", "fctr"])
    .tab_spanner("B", ["fctr"])
    .tab_spanner("C", ["num", "char"])
    .tab_spanner("D", ["fctr", "date", "time"])
    .tab_spanner("E", spanners=["B", "C"])
    # for testing
    .tab_stubhead(label="Group")

)
Screenshot 2024-05-14 at 20 21 18

@timkpaine
Copy link
Contributor Author

@machow I'm happy to write tests/docs, just need some guidance. The two examples here might be sufficient to assert that their table header structure is created correctly?

from great_tables import GT, exibble

# With stub head
(
    GT(exibble, rowname_col="row", groupname_col="group")
    .tab_spanner("A", ["num", "char", "fctr"])
    .tab_spanner("B", ["fctr"])
    .tab_spanner("C", ["num", "char"])
    .tab_spanner("D", ["fctr", "date", "time"])
    .tab_spanner("E", spanners=["B", "C"])
    # for testing
    .tab_stubhead(label="Group")
)

# Without stub head
(
    GT(exibble)
    .tab_spanner("A", ["num", "char", "fctr"])
    .tab_spanner("B", ["fctr"])
    .tab_spanner("C", ["num", "char"])
    .tab_spanner("D", ["fctr", "date", "time"])
    .tab_spanner("E", spanners=["B", "C"])
)

timkpaine added a commit to timkpaine/great-tables that referenced this pull request May 15, 2024
@timkpaine timkpaine marked this pull request as ready for review May 15, 2024 15:02
@codecov-commenter
Copy link

codecov-commenter commented May 15, 2024

Codecov Report

Attention: Patch coverage is 92.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 85.14%. Comparing base (7eae1f2) to head (c8a1889).

Files Patch % Lines
great_tables/_spanners.py 81.81% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #345      +/-   ##
==========================================
+ Coverage   83.51%   85.14%   +1.62%     
==========================================
  Files          41       41              
  Lines        4308     4316       +8     
==========================================
+ Hits         3598     3675      +77     
+ Misses        710      641      -69     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@machow
Copy link
Collaborator

machow commented May 15, 2024

@rich-iannone do you mind taking a look?

edit: the examples seem like they hit the key pieces. I'm not too familiar with the exact spanner structure, though 😅

When I pasted the snapshot into codepen (and add borders), it looked like this:

image

@machow machow requested a review from rich-iannone May 15, 2024 15:10
@rich-iannone
Copy link
Member

rich-iannone commented May 15, 2024

I did some testing and I only found one issue with this (aside from the preexisting issue #273, which could be fixed later). Here's what I used for one of the more comprehensive tests:

(
  GT(exibble_pl, rowname_col="row", groupname_col="group")
  .fmt_number(columns="num")
  .sub_missing("num")
  .cols_label(num = "Number")
  .tab_spanner(label="1st three", columns = ["num", "char", "fctr"], id="one")
  .tab_spanner(label="2nd three", columns = ["date", "time", "datetime"], id="two")
  .tab_spanner(label="Over both", columns = ["num", "char", "fctr"],  spanners="two", id="overbothspanners")
  .tab_spanner(label="3rd level", columns=["char", "num"], id="level3")
  .tab_spanner(label="4th level", columns=["char", "num"], id="level4", )
  .tab_spanner(label="Over two and currency", columns="currency", spanners="two", id="mixed", )
  .tab_spanner(label="Right above currency", columns="currency", level=0, id="over_curr")
  .tab_spanner(label="Two above currency", columns="currency", level=1)
  .tab_spanner(label="REPLACEMENT", columns="currency", level=1, replace=True)
)
spanners_tbl

This is phenomenal work because everything works as expected. The only thing that doesn't is when replace=False is used in the final .tab_spanner() call, which should result in an error. I see in the implementation that this arg is not being used and I think we could definitely punt on that one since it crosses into advanced usage.

Copy link
Member

@rich-iannone rich-iannone left a comment

Choose a reason for hiding this comment

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

Everything looks great except for the unused replace= arg. If you want to leave that til later, I'd suggest removing the arg and associated documentation for this PR.

@rich-iannone
Copy link
Member

Also, the following example fails:

from great_tables import GT, md, html
from great_tables.data import gtcars

gtcars_mini = gtcars[["mfr", "model", "year", "hp", "trq", "msrp"]].tail(10)

(
    GT(gtcars_mini, rowname_col="model", groupname_col="mfr")
    .tab_spanner(label=md("*Performance*"), columns=["hp", "trq"])
    .tab_header(
        title=html("Data listing from <strong>gtcars</strong>"),
        subtitle=html("A <span style='font-size:12px;'>small selection</span> of great cars."),
    )
    .cols_label(year="Year Produced", hp="HP", trq="Torque", msrp="Price (USD)")
    .fmt_integer(columns=["year", "hp", "trq"], use_seps=False)
    .fmt_currency(columns="msrp")
    .tab_source_note(source_note="Source: the gtcars dataset within the Great Tables package.")
)

I traced it to the validation check:

if id in crnt_spanner_ids:
        raise ValueError(f"Spanner id {id} already exists.")

Which is good, but if the id is produced from a label and the label is created with md() we get a Text object. I think we need to handle the creation of id values from Text. Maybe like this?

if id is None:
  if hasattr(label, "text"): # <- We could most probably have a better conditional stmt here
    id = label.text
  else:
    id = label

Then the example runs (but we still get bitten by #273) and produces a GT table.

Co-authored-by: Richard Iannone <riannone@me.com>
Copy link
Member

@rich-iannone rich-iannone left a comment

Choose a reason for hiding this comment

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

LGTM!

@machow machow merged commit 524d9df into posit-dev:main May 15, 2024
9 checks passed
@timkpaine timkpaine deleted the tkp/multispanner branch May 15, 2024 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spanners use 'id' as a label and not 'label'. Allow for multiple levels of spanners with tab_spanner()
4 participants