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.64% +0.19%
==========================================
Files 47 47
Lines 5558 5746 +188
==========================================
+ Hits 5083 5266 +183
- Misses 475 480 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…h(), called also for making summary rows
| row_stub_var = data._boxhead._get_stub_column() | ||
|
|
||
| stub_layout = data._stub._get_stub_layout(options=data._options) | ||
| has_summary_rows = bool(data._summary_rows or data._summary_rows_grand) |
There was a problem hiding this comment.
This expression appears several times in this PR. I'm not sure if there's a better approach that doesn't have multiple sources of truth.
has_summary_rows = bool(data._summary_rows or data._summary_rows_grand)
| summary_row_stub_var = ColInfo( | ||
| "__summary_row__", ColInfoTypeEnum.stub, column_align="left" | ||
| ) | ||
| column_vars = [summary_row_stub_var] + column_vars |
There was a problem hiding this comment.
We discussed this case today – I wonder if having a flag in the ColInfo object is worth creating, to avoid using the ColInfo.var attribute to choose the summary row.
|
|
||
|
|
||
| def max_expr(df: pd.DataFrame): | ||
| return df.max(numeric_only=True) |
There was a problem hiding this comment.
These are helpers. Does it make sense to have them here?
There was a problem hiding this comment.
I am not super confident in my testing of the new single dispatch function, eval_aggregate(). Open to feedback/changes.
| 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.
| # 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.
|
|
||
| @set_style.register | ||
| def _(loc: LocBody, data: GTData, style: list[CellStyle]) -> GTData: | ||
| # @set_style.register(LocSummary) |
|
|
||
|
|
||
| def grand_summary_rows( | ||
| self: GTSelf, |
There was a problem hiding this comment.
TODO: let's make these keyword only for now, so we can feel out final signature
| side=side, | ||
| ) | ||
|
|
||
| self._summary_rows_grand.add_summary_row(summary_row_info) |
There was a problem hiding this comment.
Let's ensure we don't mutate here and ensure summary row data objects are not allowed to mutate themselves
| ) | ||
| from ._spanners import spanners_print_matrix | ||
| from ._tbl_data import _get_cell, cast_frame_to_string, replace_null_frame | ||
| from ._tbl_data import TblData, _get_cell, cast_frame_to_string, replace_null_frame |
There was a problem hiding this comment.
Let's put TblData in a if TYPE_CHECKING block, to flag that we are not instantiating any Tbly things
| else: | ||
| cell_content = summary_row.values.get(colinfo.var) | ||
| else: | ||
| if colinfo.var == "__summary_row__": |
There was a problem hiding this comment.
TODO: double check this (used twice)
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