Feat: add grand_summary_rows() method
#765
Conversation
juleswg23
changed the title
grand_summary_rows() method
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #765 +/- ##
==========================================
+ Coverage 91.45% 91.58% +0.13%
==========================================
Files 47 47
Lines 5558 5752 +194
==========================================
+ Hits 5083 5268 +185
- Misses 475 484 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…h(), called also for making summary rows
| if columns is not None: | ||
| raise NotImplementedError( | ||
| "Currently, grand_summary_rows() does not support column selection." | ||
| ) |
There was a problem hiding this comment.
Currently we raise an error, which is documented in the docstring. Flagging in a comment just in case there's a better way to stub this out.
| def grand_summary_rows( | ||
| self: GTSelf, | ||
| fns: dict[str, PlExpr] | dict[str, Callable[[TblData], Any]], | ||
| fmt: FormatFn | None = None, |
There was a problem hiding this comment.
Having used this in a couple cases, I am leaning towards accepting a function instead of a FormatFn, the same way that fmt() can take a custom-built fn. I haven't explore the implementation details of this switch.
great_tables/_gt_data.py
Outdated
| # Replace with numeric values from new row | ||
| for key, new_value in summary_row.values.items(): | ||
| if isinstance(new_value, (int, float)): | ||
| merged_values[key] = new_value |
There was a problem hiding this comment.
I can tell this is not the ideal way, since we can't guarantee that summary values have to be numeric.
| _heading=Heading(), | ||
| _stubhead=None, | ||
| _summary_rows=SummaryRows(), | ||
| _summary_rows_grand=SummaryRows(_is_grand_summary=True), |
There was a problem hiding this comment.
It looks like _is_grand_summary is used so that for methods like __getitem__ and add_summary_rows() some arguments that are required for regular summary rows can be optional for the grand one.
There was a problem hiding this comment.
I'm going to punt on any refactoring of SummaryRows(). I'll note it seems like the list in list[SummaryRowInfo] is doing a lot of heavy lifting. For example, it can be...
- filtered by side
- appended to
- tasked with replacing an existing SummaryRowInfo with a new one (whose id is the same)
Whereas SummaryRows is mostly a dictionary (with methods in this implementation to work on those lists).
There was a problem hiding this comment.
mutations removed for now, so should be okay as is
There was a problem hiding this comment.
Alright, I think this is ready to go! I don't remember looking at the new doc pages closely (and I added one hidden doc called "technical-notes"). @rich-iannone do you mind taking a look at this / merging if it looks okay?!
There was a problem hiding this comment.
Alright, I think this is ready to go! I don't remember looking at the new doc pages closely (and I added one hidden doc called "technical-notes"). @rich-iannone do you mind taking a look at this / merging if it looks okay?!
There was a problem hiding this comment.
Alright, I think this is ready to go! I don't remember looking at the new doc pages closely (and I added one hidden doc called "technical-notes"). @rich-iannone do you mind taking a look at this / merging if it looks okay?!
|
Thanks so much @juleswg23 and @machow for your work on this. I'll review shortly and merge if everything is looking good! |
3 participants
Summary
Work in Progress for new feature, grand summary rows
This PR summary will be updated
Related GitHub Issues and PRs
grand_summary_rows()#173, possibly epic: Implementsummary_rows()#172Checklist