-
Notifications
You must be signed in to change notification settings - Fork 0
feat: Introduce new summary section for data metrics #47
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,17 +5,24 @@ | |
|
|
||
| from collections.abc import Callable | ||
| from dataclasses import dataclass | ||
| from typing import Literal | ||
|
|
||
| import polars as pl | ||
| import polars.selectors as cs | ||
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class Metric: | ||
| """A metric function paired with a column-applicability selector.""" | ||
| """A metric function paired with a column-applicability selector. | ||
|
|
||
| ``kind`` selects the summary section the metric is rendered in: ``"change"`` metrics | ||
| appear as columns in the "Columns" table, while ``"data"`` metrics get their own | ||
| "Data Inspection" section. | ||
| """ | ||
|
|
||
| fn: MetricFn | ||
| selector: cs.Selector | ||
| kind: Literal["change", "data"] = "change" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it could be good to not provide a default here to force the user to think about what kind of metric they implemented. Wdyt?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, nothing prevents me from "registering" a change metric as a data metric and vice versa. Going a step further, I wish there was a way to infer DataMetricFn = Callable[[pl.Expr], pl.Expr]and then rewrite: def null_fraction_change(col: pl.Expr) -> pl.Expr:
return col.is_null().mean()
class Metric:
fn: ChangeMetricFn | DataMetricFn
#(property)
def kind(self) -> str:
return "data" if isinstance(DataMetricFn, self.fn) else "change"
|
||
|
|
||
|
|
||
| MetricFn = Callable[[pl.Expr, pl.Expr], pl.Expr] | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,11 @@ | |
| from ._common import Metric, MetricFn | ||
|
|
||
|
|
||
| def _make_data_metric(fn: MetricFn, selector: cs.Selector = cs.all()) -> Metric: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO this partial method does not add a lot of value, so it could be dropped. We could just add the |
||
| """Wrap a metric function as a data metric, applicable to all columns by default.""" | ||
| return Metric(fn=fn, selector=selector, kind="data") | ||
|
|
||
|
|
||
| def null_fraction_change(left: pl.Expr, right: pl.Expr) -> pl.Expr: | ||
| """Change in the fraction of null entries, rendered as ``<old> -> <new> (<delta>)``. | ||
|
|
||
|
|
@@ -35,7 +40,7 @@ def null_fraction_change(left: pl.Expr, right: pl.Expr) -> pl.Expr: | |
|
|
||
|
|
||
| DEFAULT_DATA_METRICS: dict[str, MetricFn | Metric] = { | ||
| "Null%": Metric(fn=null_fraction_change, selector=cs.all()), | ||
| "Null%": _make_data_metric(null_fraction_change), | ||
| } | ||
| """Preset metrics describing the left and right datasets individually.""" | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -131,7 +131,9 @@ def to_json(self, **kwargs: Any) -> str: | |||||
| "name": "value", | ||||||
| "match_rate": 0.667, | ||||||
| "n_total_changes": 1, | ||||||
| "changes": [{"old": 1.0, "new": 2.0, "count": 1, "sample_pk": [1]}] | ||||||
| "changes": [{"old": 1.0, "new": 2.0, "count": 1, "sample_pk": [1]}], | ||||||
| "metrics": null, | ||||||
| "data_metrics": null | ||||||
| } | ||||||
| ], | ||||||
| "sample_rows_left_only": [], | ||||||
|
|
@@ -179,6 +181,7 @@ def _print_diff(self, console: Console) -> None: | |||||
| self._print_schemas(console) | ||||||
| self._print_rows(console) | ||||||
| self._print_columns(console) | ||||||
| self._print_data_inspection(console) | ||||||
| self._print_sample_rows_only_one_side(console, side=Side.LEFT) | ||||||
| self._print_sample_rows_only_one_side(console, side=Side.RIGHT) | ||||||
|
|
||||||
|
|
@@ -571,7 +574,7 @@ def _section_columns(self) -> RenderableType: | |||||
| elif not columns: | ||||||
| display_items.append(Text("All columns match perfectly.", style="italic")) | ||||||
| else: | ||||||
| metric_labels = self._data._metric_labels | ||||||
| metric_labels = self._data._change_metric_labels | ||||||
| matches = Table(show_header=bool(metric_labels)) | ||||||
|
MoritzPotthoffQC marked this conversation as resolved.
|
||||||
| matches.add_column( | ||||||
| "Column", | ||||||
|
|
@@ -632,6 +635,36 @@ def _section_columns(self) -> RenderableType: | |||||
|
|
||||||
| return Group(*display_items) | ||||||
|
|
||||||
| # ------------------------------- DATA INSPECTION -------------------------------- # | ||||||
|
|
||||||
| def _print_data_inspection(self, console: Console) -> None: | ||||||
| if not self._data.columns or not self._data._data_metric_labels: | ||||||
| return | ||||||
| _print_section( | ||||||
| console, | ||||||
| "Data Inspection", | ||||||
| self._section_data_inspection(), | ||||||
| ) | ||||||
|
|
||||||
| def _section_data_inspection(self) -> RenderableType: | ||||||
| assert self._data.columns is not None | ||||||
|
|
||||||
| table = Table() | ||||||
| table.add_column( | ||||||
| "Column", | ||||||
| max_width=COLUMN_SECTION_COLUMN_WIDTH, | ||||||
| overflow=OVERFLOW, | ||||||
| ) | ||||||
| for label in self._data._data_metric_labels: | ||||||
| table.add_column(label, justify="right", overflow=OVERFLOW) | ||||||
| for col in self._data.columns: | ||||||
| row_items: list[RenderableType] = [Text(col.name, style="cyan")] | ||||||
| for label in self._data._data_metric_labels: | ||||||
| value = col.data_metrics.get(label) if col.data_metrics else None | ||||||
| row_items.append(_format_metric_value(value)) | ||||||
| table.add_row(*row_items) | ||||||
| return table | ||||||
|
|
||||||
| # ------------------------------ ROWS ONLY ONE SIDE ------------------------------ # | ||||||
|
|
||||||
| def _print_sample_rows_only_one_side(self, console: Console, side: Side) -> None: | ||||||
|
|
@@ -716,6 +749,7 @@ class SummaryDataColumn: | |||||
| n_total_changes: int | ||||||
| changes: list[SummaryDataColumnChange] | None | ||||||
| metrics: dict[str, Any] | None | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ?
Suggested change
|
||||||
| data_metrics: dict[str, Any] | None | ||||||
|
|
||||||
|
|
||||||
| @dataclass | ||||||
|
|
@@ -733,7 +767,8 @@ class SummaryData: | |||||
| _other_common_columns: list[str] | ||||||
| _truncated_left_name: str | ||||||
| _truncated_right_name: str | ||||||
| _metric_labels: list[str] | ||||||
| _change_metric_labels: list[str] | ||||||
| _data_metric_labels: list[str] | ||||||
|
|
||||||
| def to_dict(self) -> dict[str, Any]: | ||||||
| def _convert(obj: Any) -> Any: | ||||||
|
|
@@ -836,12 +871,18 @@ def _validate_primary_key_hidden_columns() -> None: | |||||
| _other_common_columns=comp._other_common_columns, | ||||||
| _truncated_left_name=truncated_left, | ||||||
| _truncated_right_name=truncated_right, | ||||||
| _metric_labels=[], | ||||||
| _change_metric_labels=[], | ||||||
| _data_metric_labels=[], | ||||||
| ) | ||||||
|
|
||||||
| metrics_resolved: dict[str, Metric] = dict(metrics or {}) | ||||||
| metrics_by_column = _compute_column_metrics(comp, metrics_resolved) | ||||||
| metric_labels = list(metrics_resolved.keys()) | ||||||
| change_metric_labels = [ | ||||||
| label for label, m in metrics_resolved.items() if m.kind == "change" | ||||||
| ] | ||||||
| data_metric_labels = [ | ||||||
| label for label, m in metrics_resolved.items() if m.kind == "data" | ||||||
| ] | ||||||
|
|
||||||
| schemas = _compute_schemas(comp, slim) | ||||||
| rows = _compute_rows(comp, slim) | ||||||
|
|
@@ -852,6 +893,8 @@ def _validate_primary_key_hidden_columns() -> None: | |||||
| top_k_changes_by_column, | ||||||
| show_sample_primary_key_per_change, | ||||||
| metrics_by_column, | ||||||
| change_metric_labels, | ||||||
| data_metric_labels, | ||||||
| ) | ||||||
| sample_rows_left_only, sample_rows_right_only = _compute_sample_rows( | ||||||
| comp, sample_k_rows_only | ||||||
|
|
@@ -871,7 +914,8 @@ def _validate_primary_key_hidden_columns() -> None: | |||||
| _other_common_columns=comp._other_common_columns, | ||||||
| _truncated_left_name=truncated_left, | ||||||
| _truncated_right_name=truncated_right, | ||||||
| _metric_labels=metric_labels, | ||||||
| _change_metric_labels=change_metric_labels, | ||||||
| _data_metric_labels=data_metric_labels, | ||||||
| ) | ||||||
|
|
||||||
|
|
||||||
|
|
@@ -977,6 +1021,8 @@ def _compute_columns( | |||||
| top_k_changes_by_column: dict[str, int], | ||||||
| show_sample_primary_key_per_change: bool, | ||||||
| metrics_by_column: dict[str, dict[str, Any]], | ||||||
| change_metric_labels: list[str], | ||||||
| data_metric_labels: list[str], | ||||||
| ) -> list[SummaryDataColumn] | None: | ||||||
| # NOTE: We can only compute column matches if there are primary key columns and at | ||||||
| # least one joined row. | ||||||
|
|
@@ -1017,13 +1063,25 @@ def _compute_columns( | |||||
| sample_pk=sample_pk, | ||||||
| ) | ||||||
| ) | ||||||
| col_metrics = metrics_by_column.get(col_name) or {} | ||||||
| change_metrics = { | ||||||
| label: col_metrics[label] | ||||||
| for label in change_metric_labels | ||||||
| if label in col_metrics | ||||||
| } | ||||||
| data_metrics = { | ||||||
| label: col_metrics[label] | ||||||
| for label in data_metric_labels | ||||||
| if label in col_metrics | ||||||
| } | ||||||
| columns.append( | ||||||
| SummaryDataColumn( | ||||||
| name=col_name, | ||||||
| match_rate=rate, | ||||||
| n_total_changes=n_total_changes, | ||||||
| changes=changes, | ||||||
| metrics=metrics_by_column.get(col_name), | ||||||
| metrics=change_metrics or None, | ||||||
| data_metrics=data_metrics or None, | ||||||
| ) | ||||||
| ) | ||||||
| return columns | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it could be nicer to also explain the semantic difference between change metrics and data metrics. Right now, it sounds like the only difference is where it gets rendered, but the underlying reason for why we present these two options is not clear.