feat: add `cols_label_with()` method #626

jrycw · 2025-03-02T18:07:44Z

Hello team,

This PR ports the cols_label_with() function from {gt} to Great Tables.

codecov · 2025-03-02T18:09:01Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.24%. Comparing base (dc82197) to head (ce5d5f1).
Report is 9 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #626      +/-   ##
==========================================
+ Coverage   90.85%   91.24%   +0.39%     
==========================================
  Files          46       47       +1     
  Lines        5423     5484      +61     
==========================================
+ Hits         4927     5004      +77     
+ Misses        496      480      -16

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

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

rich-iannone

LGTM!

jrycw · 2025-03-03T10:46:06Z

Whoops! I'm unsure whether we should call cols_label() instead of directly using Boxhead._set_column_labels(), as cols_label() includes additional logic for handling unit syntax.

jrycw · 2025-03-03T12:52:43Z

Alternatively, we could extract this code block into a separate function, allowing both cols_label() and cols_label_with() to call it.

great-tables/great_tables/_boxhead.py

Lines 134 to 152 in dc82197

    
           # Handle units syntax in labels (e.g., "Density ({{ppl / mi^2}})") 
        
           new_kwargs: dict[str, UnitStr | str | BaseText] = {} 
        
           for k, v in new_cases.items(): 
        
               if isinstance(v, str): 
        
                   unitstr_v = UnitStr.from_str(v) 
        
                   if len(unitstr_v.units_str) == 1 and isinstance(unitstr_v.units_str[0], str): 
        
                       new_kwargs[k] = unitstr_v.units_str[0] 
        
                   else: 
        
                       new_kwargs[k] = unitstr_v 
        
               elif isinstance(v, BaseText): 
        
                   new_kwargs[k] = v 
        
               else: 
        
                   raise ValueError( 
        
                       "Column labels must be strings or BaseText objects. Use `md()` or `html()` for formatting." 
        
                   )

rich-iannone · 2025-03-03T16:14:20Z

Thanks for considering the units syntax codepath (I missed that too)! For incorporating that functionality, I do think your last comment (create a units handler as a shared function) is the way right to go.

rich-iannone · 2025-03-03T16:15:59Z

As for the docs-building CI error, we solved that in a recent commit by removing the pip install of the pointblank package in the CI workflow file. Just merge with main and you should be all set!

…tax()`

rich-iannone

LGTM!

machow

Thanks for this PR -- this function seems really helpful! WDYT of making columns the first argument? I detail a bit of the rationale in a comment.

Here's a detail that's not critical to this PR

One thing I learned just now going through the R library, is that there cols_label has a .fn= parameter for doing these kinds of transformations. However, it sounds like cols_label_with() is a helpful convenience function, because this is something people often want to do.

Essentially, the breakdown seems like:

cols_label() often involves directly relabeling columns (e.g. a = "b").
cols_label_with() only allows transformation via a function.

However, because we haven't added cols_label(fn=...), there's an opportunity in the future to define it. One wild idea, would be supporting Polars selectors somehow. E.g.

import polars as pl
import polars.selectors as cs

# imagine that selectors are run over a special DataFrame w/ 1 row
df_names = pl.DataFrame({"my_col": ["my_col"], "my_col2": ["my_col2"]})

# the selected column names are the columns to relabel, the value in the single
# row is the new name. There might be a more natural way to do this with
# polars, but wanted to float it as a broad idea!
df_names.select(cs.starts_with("my").str.to_uppercase())

machow · 2025-03-06T17:59:26Z

great_tables/_boxhead.py

+    return self._replace(_boxhead=boxhead)
+
+
+def cols_label_with(self: GTSelf, fn: Callable[[str], str], columns: SelectExpr = None) -> GTSelf:


From pairing w/ Rich, @jrycw WDYT of switching the order of the parameters, so columns is before fn?

Looking through the cols_*() methods in the R library, column selection usually comes first. In the case of cols_align() where it doesn't, it sounds like it's a historical artifact. Keeping column first might help cement a pattern for cols_*() methods.

It makes sense you put it first in the PR, since it doesn't have a default. WDYT of us setting it to fn = None, and then erroring if fn is None?

@machow, nice suggestion! I definitely like your idea of introducing fn= to cols_label() using Polars syntax.

Thanks to @machow for the comment! I think this is a good opportunity to introduce fn= to accept a callable in cols_label() as well.

By the way, I noticed that we have two test files for boxhead, test_boxhead.py and test__boxhead.py. I picked one to add the new test, but perhaps we should consider consolidating them in the future.

As a side note, we might consider using the pl.Expr.name attribute to handle Polars syntax. For example:

df_names.select(cs.starts_with("my").name.to_uppercase())

shape: (1, 2) ┌────────┬─────────┐ │ MY_COL ┆ MY_COL2 │ │ --- ┆ --- │ │ str ┆ str │ ╞════════╪═════════╡ │ my_col ┆ my_col2 │ └────────┴─────────┘

Well, I've attempted to implement this in the latest commit. I admit it's a bit of a bold move, so feel free to set it aside for now.

Here’s a simple example:

import polars as pl import polars.selectors as cs from great_tables import GT df_names = pl.DataFrame({"my_col": ["my_col"], "my_col2": ["my_col2"]}) GT(df_names).cols_label_with(converter=cs.starts_with("my").name.to_uppercase())

When passing a list of Polars expressions, if a column is referenced multiple times, we can either superimpose the transformations or follow a "last one wins" approach (current implementation).

Thanks for experimenting with this. If it's okay with you, I think we need a little bit more time to chew on this approach / run it past some folks, since it's a bit cutting edge in terms of external libraries using Polars selectors. I definitely think it'll be useful to figure out in the long run though

…l_with()` based on feedback

machow

Thanks so much for working my feedback in. I left comments asking if we could remove fn from .cols_label() and remove unit string processing from .cols_label_with().

I'm still getting a feel for some dynamics in these functions, so thanks for being so patient on this PR 😅

machow · 2025-03-07T18:12:26Z

great_tables/_boxhead.py

-    self: GTSelf, cases: dict[str, str | BaseText] | None = None, **kwargs: str | BaseText
+    self: GTSelf,
+    cases: dict[str, str | BaseText] | None = None,
+    fn: Callable[[str | BaseText], str | BaseText] | None = None,


Thanks for taking this and running with it. I'm so sorry to send you down a rough path, but maybe we take fn out of here for now?

We're realizing that the combination of fn= and the ability to wrap inputs with BaseText containers is a bit tricky. Since we'd have to explain and figure out how to apply fn to things like md("**some text**").

This seems resolvable, but I would punt on for the sake of getting cols_label_with() in. There's some cleanup across Great Tables we can likely do with BaseText and UnitStr/definition list, which I'm guessing will make situations like this easier to handle.

machow · 2025-03-07T18:15:36Z

great_tables/_boxhead.py

+    new_cases = {col: fn(col) for col in sel_cols}
+
+    # Handle units syntax in labels (e.g., "Density ({{ppl / mi^2}})")
+    new_kwargs = _handle_units_syntax(new_cases)


It looks like the R package doesn't automatically apply unit syntax here. My understanding is that this makes .cols_label_with() a very plain custom function applier. WDYT of not handling unit syntax here?

We could tell people they can make it happen with define_units() etc.. so hopefully they'd be able to make it happen if they need it

I'm still getting a feel for some of these dynamics, so could be totally off

@machow, no worries—happy to work on this. Regarding define_units(), I’m having some difficulty describing it since its definition and use cases are still a bit unclear to me (e.g., #446). Maybe you could help clarify this in the documentation.

Shoot, I can see how this is confusing. I'll take a look at the linked PR, and think about what might put units more front and center. I think it's okay for now to just omit any mention of units in this docstring.

…fect the result

…_label_with()`

machow

@jrycw there are a lot of really interesting changes here, but I think this PR has a bit more in it than I can review.

When I first reviewed, I'd have been onboard with merging the PR after just the columns and fn arguments were switched (so columns was first). If you could revert to that, and make that change (plus having .cols_label_with() not do any unit syntax handling, I'm happy to merge.

(The extra expression stuff is exciting, and seems really useful down the road, but it's a lot to review at once. I do think there's a lot of interesting Polars logic in the PR right now for handling expressions. If you want to stash it somewhere, it could also go into an issue?)

#626 (comment)

jrycw · 2025-03-25T00:26:32Z

@jrycw there are a lot of really interesting changes here, but I think this PR has a bit more in it than I can review.

When I first reviewed, I'd have been onboard with merging the PR after just the columns and fn arguments were switched (so columns was first). If you could revert to that, and make that change (plus having .cols_label_with() not do any unit syntax handling, I'm happy to merge.

(The extra expression stuff is exciting, and seems really useful down the road, but it's a lot to review at once. I do think there's a lot of interesting Polars logic in the PR right now for handling expressions. If you want to stash it somewhere, it could also go into an issue?)

#626 (comment)

@machow, thanks for the feedback. I'm not sure which commit you're referring to, but feel free to pick any relevant one for the revert. Could you help proceed with the revert process? Meanwhile, I've created issue #648 to track the Polars expression implementation.

jrycw added 3 commits March 3, 2025 02:05

Implement cols_label_with()

d53ae18

Add tests for cols_label_with()

43766cc

Include cols_label_with() in _quarto.yml

f89e19b

jrycw requested review from rich-iannone and machow as code owners March 2, 2025 18:07

rich-iannone approved these changes Mar 2, 2025

View reviewed changes

jrycw added 2 commits March 4, 2025 00:51

Extract unit handling logic for column labels into `_handle_units_syn…

22d93b1

…tax()`

Add tests for _handle_units_syntax()

ee637b8

rich-iannone approved these changes Mar 3, 2025

View reviewed changes

machow requested changes Mar 6, 2025

View reviewed changes

jrycw added 3 commits March 7, 2025 10:04

Switch the positions of columns= and fn= parameters in `cols_labe…

2852f26

…l_with()` based on feedback

Add fn= parameter to cols_label()

1ae6f8a

Update type hint for fn= in cols_label()

2ee205a

machow requested changes Mar 7, 2025

View reviewed changes

jrycw added 7 commits March 8, 2025 10:03

Revert "Add fn= parameter to cols_label()

a060849

Avoid automatic unit syntax handling in cols_label_with()

73c08f9

Allow passing a Polars expression to converter= in cols_label_with()

b3564d2

Update the example for cols_label_with()

1d4b4e6

Add a test for cols_label_with() to ensure column order does not af…

25e661c

…fect the result

Support passing a list of Polars expressions to converter= in `cols…

50542f8

…_label_with()`

Simplify test_handle_units_syntax()

ce5d5f1

machow requested changes Mar 24, 2025

View reviewed changes

jrycw mentioned this pull request Mar 25, 2025

Allow passing a Polars expression to change labels in cols_label_with() #648

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add `cols_label_with()` method #626

feat: add `cols_label_with()` method #626

jrycw commented Mar 2, 2025

codecov bot commented Mar 2, 2025 •

edited

Loading

rich-iannone left a comment

jrycw commented Mar 3, 2025

jrycw commented Mar 3, 2025

rich-iannone commented Mar 3, 2025

rich-iannone commented Mar 3, 2025

rich-iannone left a comment

machow left a comment

machow Mar 6, 2025

jrycw Mar 7, 2025

jrycw Mar 7, 2025

jrycw Mar 9, 2025 •

edited

Loading

jrycw Mar 9, 2025

jrycw Mar 9, 2025

jrycw Mar 9, 2025 •

edited

Loading

machow Mar 11, 2025

machow left a comment

machow Mar 7, 2025

machow Mar 7, 2025

jrycw Mar 8, 2025 •

edited

Loading

machow Mar 11, 2025

machow left a comment

jrycw commented Mar 25, 2025

		return self._replace(_boxhead=boxhead)


		def cols_label_with(self: GTSelf, fn: Callable[[str], str], columns: SelectExpr = None) -> GTSelf:

feat: add cols_label_with() method #626

Are you sure you want to change the base?

feat: add cols_label_with() method #626

Conversation

jrycw commented Mar 2, 2025

codecov bot commented Mar 2, 2025 • edited Loading

Codecov Report

rich-iannone left a comment

Choose a reason for hiding this comment

jrycw commented Mar 3, 2025

jrycw commented Mar 3, 2025

rich-iannone commented Mar 3, 2025

rich-iannone commented Mar 3, 2025

rich-iannone left a comment

Choose a reason for hiding this comment

machow left a comment

Choose a reason for hiding this comment

Here's a detail that's not critical to this PR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrycw Mar 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrycw Mar 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

machow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrycw Mar 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

machow left a comment

Choose a reason for hiding this comment

jrycw commented Mar 25, 2025

feat: add `cols_label_with()` method #626

feat: add `cols_label_with()` method #626

codecov bot commented Mar 2, 2025 •

edited

Loading

jrycw Mar 9, 2025 •

edited

Loading

jrycw Mar 9, 2025 •

edited

Loading

jrycw Mar 8, 2025 •

edited

Loading