feat: Generalize metrics and add nullability delta metric#43
Open
Moritz Potthoff (MoritzPotthoffQC) wants to merge 9 commits into
Open
feat: Generalize metrics and add nullability delta metric#43Moritz Potthoff (MoritzPotthoffQC) wants to merge 9 commits into
Moritz Potthoff (MoritzPotthoffQC) wants to merge 9 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #43 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 11 11
Lines 1018 1030 +12
=========================================
+ Hits 1018 1030 +12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This was referenced Jul 10, 2026
Merged
Copilot started reviewing on behalf of
Moritz Potthoff (MoritzPotthoffQC)
July 10, 2026 14:28
View session
There was a problem hiding this comment.
Pull request overview
This PR extends Diffly’s per-column summary metrics to support non-numeric columns via an explicit Metric(fn, selector=...) wrapper, and introduces a built-in metric for nullability changes so users can quickly see how null fractions evolve between left/right frames.
Changes:
- Allow
summary(metrics=...)/ testing helpers to accept either a bare metric callable (numeric-only) or aMetricwith a Polars selector (any column types). - Add the
null_fraction_changepreset and expose it asΔNull%inDEFAULT_METRICSand the CLI. - Update summary formatting + docs, and add tests + generated summary fixtures for the new metric behavior.
Reviewed changes
Copilot reviewed 41 out of 41 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| diffly/metrics.py | Adds Metric wrapper type, null-fraction change preset, and updates DEFAULT_METRICS to include selector-based metrics. |
| diffly/comparison.py | Accepts Metric objects in summary(metrics=...) and resolves bare callables as numeric-only. |
| diffly/summary.py | Allows metrics to render string results in column summaries. |
| diffly/testing.py | Broadens metrics typing/docs to allow Metric values in assertion helpers. |
| diffly/cli.py | Updates CLI help text and passes selector-based preset metrics through to summary(). |
| docs/api/metrics.rst | Documents selector-based metrics and adds Metric + null_fraction_change to the API docs. |
| tests/test_metrics.py | Adds unit tests for null_fraction_change output (including non-numeric columns). |
| tests/cli/test_cli.py | Adds CLI coverage for selecting the ΔNull% metric preset. |
| tests/summary/fixtures/metrics_null_fraction/test_metrics_null_fraction.py | New fixture generator test for summaries including ΔNull% and a selector-based custom metric. |
| tests/summary/fixtures/metrics_null_fraction/gen/pretty_True_perfect_True_top_True_slim_True_sample_rows_True_sample_pk_True.txt | Generated summary fixture variant for ΔNull%. |
| tests/summary/fixtures/metrics_null_fraction/gen/pretty_True_perfect_True_top_True_slim_True_sample_rows_False_sample_pk_False.txt | Generated summary fixture variant for ΔNull%. |
| tests/summary/fixtures/metrics_null_fraction/gen/pretty_True_perfect_True_top_True_slim_False_sample_rows_True_sample_pk_True.txt | Generated summary fixture variant for ΔNull%. |
| tests/summary/fixtures/metrics_null_fraction/gen/pretty_True_perfect_True_top_True_slim_False_sample_rows_False_sample_pk_False.txt | Generated summary fixture variant for ΔNull%. |
| tests/summary/fixtures/metrics_null_fraction/gen/pretty_True_perfect_True_top_False_slim_True_sample_rows_True_sample_pk_False.txt | Generated summary fixture variant for ΔNull%. |
| tests/summary/fixtures/metrics_null_fraction/gen/pretty_True_perfect_True_top_False_slim_True_sample_rows_False_sample_pk_False.txt | Generated summary fixture variant for ΔNull%. |
| tests/summary/fixtures/metrics_null_fraction/gen/pretty_True_perfect_True_top_False_slim_False_sample_rows_True_sample_pk_False.txt | Generated summary fixture variant for ΔNull%. |
| tests/summary/fixtures/metrics_null_fraction/gen/pretty_True_perfect_True_top_False_slim_False_sample_rows_False_sample_pk_False.txt | Generated summary fixture variant for ΔNull%. |
| tests/summary/fixtures/metrics_null_fraction/gen/pretty_True_perfect_False_top_True_slim_True_sample_rows_True_sample_pk_True.txt | Generated summary fixture variant for ΔNull%. |
| tests/summary/fixtures/metrics_null_fraction/gen/pretty_True_perfect_False_top_True_slim_True_sample_rows_False_sample_pk_False.txt | Generated summary fixture variant for ΔNull%. |
| tests/summary/fixtures/metrics_null_fraction/gen/pretty_True_perfect_False_top_True_slim_False_sample_rows_True_sample_pk_True.txt | Generated summary fixture variant for ΔNull%. |
| tests/summary/fixtures/metrics_null_fraction/gen/pretty_True_perfect_False_top_True_slim_False_sample_rows_False_sample_pk_False.txt | Generated summary fixture variant for ΔNull%. |
| tests/summary/fixtures/metrics_null_fraction/gen/pretty_True_perfect_False_top_False_slim_True_sample_rows_True_sample_pk_False.txt | Generated summary fixture variant for ΔNull%. |
| tests/summary/fixtures/metrics_null_fraction/gen/pretty_True_perfect_False_top_False_slim_True_sample_rows_False_sample_pk_False.txt | Generated summary fixture variant for ΔNull%. |
| tests/summary/fixtures/metrics_null_fraction/gen/pretty_True_perfect_False_top_False_slim_False_sample_rows_True_sample_pk_False.txt | Generated summary fixture variant for ΔNull%. |
| tests/summary/fixtures/metrics_null_fraction/gen/pretty_True_perfect_False_top_False_slim_False_sample_rows_False_sample_pk_False.txt | Generated summary fixture variant for ΔNull%. |
| tests/summary/fixtures/metrics_null_fraction/gen/pretty_False_perfect_True_top_True_slim_True_sample_rows_True_sample_pk_True.txt | Generated summary fixture variant for ΔNull%. |
| tests/summary/fixtures/metrics_null_fraction/gen/pretty_False_perfect_True_top_True_slim_True_sample_rows_False_sample_pk_False.txt | Generated summary fixture variant for ΔNull%. |
| tests/summary/fixtures/metrics_null_fraction/gen/pretty_False_perfect_True_top_True_slim_False_sample_rows_True_sample_pk_True.txt | Generated summary fixture variant for ΔNull%. |
| tests/summary/fixtures/metrics_null_fraction/gen/pretty_False_perfect_True_top_True_slim_False_sample_rows_False_sample_pk_False.txt | Generated summary fixture variant for ΔNull%. |
| tests/summary/fixtures/metrics_null_fraction/gen/pretty_False_perfect_True_top_False_slim_True_sample_rows_True_sample_pk_False.txt | Generated summary fixture variant for ΔNull%. |
| tests/summary/fixtures/metrics_null_fraction/gen/pretty_False_perfect_True_top_False_slim_True_sample_rows_False_sample_pk_False.txt | Generated summary fixture variant for ΔNull%. |
| tests/summary/fixtures/metrics_null_fraction/gen/pretty_False_perfect_True_top_False_slim_False_sample_rows_True_sample_pk_False.txt | Generated summary fixture variant for ΔNull%. |
| tests/summary/fixtures/metrics_null_fraction/gen/pretty_False_perfect_True_top_False_slim_False_sample_rows_False_sample_pk_False.txt | Generated summary fixture variant for ΔNull%. |
| tests/summary/fixtures/metrics_null_fraction/gen/pretty_False_perfect_False_top_True_slim_True_sample_rows_True_sample_pk_True.txt | Generated summary fixture variant for ΔNull%. |
| tests/summary/fixtures/metrics_null_fraction/gen/pretty_False_perfect_False_top_True_slim_True_sample_rows_False_sample_pk_False.txt | Generated summary fixture variant for ΔNull%. |
| tests/summary/fixtures/metrics_null_fraction/gen/pretty_False_perfect_False_top_True_slim_False_sample_rows_True_sample_pk_True.txt | Generated summary fixture variant for ΔNull%. |
| tests/summary/fixtures/metrics_null_fraction/gen/pretty_False_perfect_False_top_True_slim_False_sample_rows_False_sample_pk_False.txt | Generated summary fixture variant for ΔNull%. |
| tests/summary/fixtures/metrics_null_fraction/gen/pretty_False_perfect_False_top_False_slim_True_sample_rows_True_sample_pk_False.txt | Generated summary fixture variant for ΔNull%. |
| tests/summary/fixtures/metrics_null_fraction/gen/pretty_False_perfect_False_top_False_slim_True_sample_rows_False_sample_pk_False.txt | Generated summary fixture variant for ΔNull%. |
| tests/summary/fixtures/metrics_null_fraction/gen/pretty_False_perfect_False_top_False_slim_False_sample_rows_True_sample_pk_False.txt | Generated summary fixture variant for ΔNull%. |
| tests/summary/fixtures/metrics_null_fraction/gen/pretty_False_perfect_False_top_False_slim_False_sample_rows_False_sample_pk_False.txt | Generated summary fixture variant for ΔNull%. |
| tests/summary/fixtures/metrics_null_fraction/gen/pretty_True_perfect_True_top_True_slim_True_sample_rows_False_sample_pk_False.txt | Generated summary fixture variant for ΔNull%. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Moritz Potthoff (MoritzPotthoffQC)
July 10, 2026 14:40
View session
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Moritz Potthoff (MoritzPotthoffQC)
July 10, 2026 15:35
View session
Moritz Potthoff (MoritzPotthoffQC)
requested review from
EgeKaraismailogluQC,
Marius Merkle (MariusMerkleQC) and
Oliver Borchert (borchero)
as code owners
EgeKaraismailogluQC
left a comment
EgeKaraismailogluQC
left a comment
Collaborator
There was a problem hiding this comment.
Thank you Moritz Potthoff (@MoritzPotthoffQC), this was pleasant to review 😄 I only have some cosmetic suggestions, but also one question regarding the definition of the Null% metric:
The current definition compares the mean Null%s to each other. An alternative definition would be to check this on a per-row basis, i.e. to check how often a row that was Null is now not Null, and vice versa. When implementing the metrics feature, I rather had this second variant in mind since it makes use of the matching between each pair of rows, but the variant you implemented is of course also legitimate. My question: is this alternative definition potentially also interesting for your use case, or are you only interested in the variant which you implemented in this PR?
|
|
||
| ``old`` and ``new`` are the null percentages of the left and right side, and | ||
| ``delta`` is their signed difference (``+`` when the right side has proportionally | ||
| more nulls, ``-`` when it has fewer). Unlike the other presets, this applies to |
Collaborator
There was a problem hiding this comment.
If we add other general metrics in the future, this part will become stale and potentially confusing.
Suggested change
| more nulls, ``-`` when it has fewer). Unlike the other presets, this applies to | |
| more nulls, ``-`` when it has fewer). This metric function applies to |
|
|
||
| ``format_value(value, signed)`` formats a value for display; ``old`` and ``new`` are | ||
| rendered unsigned and the delta ``new - old`` is rendered signed (with an explicit | ||
| ``+`` or ``-`` prefix). |
Collaborator
There was a problem hiding this comment.
Negative values are always signed, right?
Suggested change
| ``+`` or ``-`` prefix). | |
| ``+`` prefix for positive values). |
Comment on lines
+70
to
+86
| def _percentage_string( | ||
| fraction: pl.Expr, *, signed: bool = False, percent_sign: bool = True | ||
| ) -> pl.Expr: | ||
| """Format a fraction as a percentage string, optionally with an explicit sign.""" | ||
| pct = (fraction * 100).round(2) | ||
| body = pl.format("{}%", pct) if percent_sign else pl.format("{}", pct) | ||
| if signed: | ||
| return pl.when(pct >= 0).then(pl.format("+{}", body)).otherwise(body) | ||
| return body | ||
|
|
||
|
|
||
| def _render_change( | ||
| old: pl.Expr, | ||
| new: pl.Expr, | ||
| format_value: Callable[[pl.Expr, bool], pl.Expr], | ||
| ) -> pl.Expr: | ||
| """Render a change as ``<old> -> <new> (<delta>)``. |
Collaborator
There was a problem hiding this comment.
Did you define these two helper methods at the top level so that they can be used by other metrics in the future? If so, I like it! Can you please move these two (and I guess also _make_numeric_metric) to the bottom of this file and add a comment divider, e.g.
# ------------------------------------------------------------------------------------ #
# UTILITY METHODS #
# ------------------------------------------------------------------------------------ #?
Comment on lines
+13
to
+15
| A bare callable is only computed for numerical columns. To target other column | ||
| types, wrap it in a :class:`Metric` with a column selector, e.g. | ||
| ``Metric(fn, selector=cs.string())`` or ``selector=cs.all()``. |
Collaborator
There was a problem hiding this comment.
It could help to give more examples here. I personally find the use case where the user selects a specific column quite interesting and worth knowing:
Suggested change
| A bare callable is only computed for numerical columns. To target other column | |
| types, wrap it in a :class:`Metric` with a column selector, e.g. | |
| ``Metric(fn, selector=cs.string())`` or ``selector=cs.all()``. | |
| A bare callable is only computed for numerical columns. To target a different | |
| set of columns, wrap it in a :class:`Metric` with a column selector, e.g. | |
| ``Metric(fn, selector=cs.all())``, ``Metric(fn, selector=cs.boolean())``, or | |
| ``Metric(fn, selector=cs.by_name("my_column_name"))``. |
| assert result.exit_code == 0 | ||
|
|
||
|
|
||
| def test_cli_null_fraction_metric(tmp_path: Path) -> None: |
Collaborator
There was a problem hiding this comment.
Since we don't have a test for the other metrics here, I think we can drop this one as well 😄
| metrics={ | ||
| # Numeric-only preset alongside a metric applied to all columns. | ||
| "Mean": metrics.mean, | ||
| "Null%": metrics.DEFAULT_METRICS["Null%"], |
Collaborator
There was a problem hiding this comment.
?
Suggested change
| "Null%": metrics.DEFAULT_METRICS["Null%"], | |
| "Null%": metrics.null_fraction_change, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
I like the concept of metrics to give the user as much information about their data at first glance. I think it would be nice to extend it even further by generalizing metrics to non-numeric columns. The specific use-case I have for this is to explain how the share of null values in a column changed.
Changes
<old> -> <new> (<delta>). This is consistent with the sample values we show. feat: Improve existing numeric metrics for new output format #44 also applies this to existing metrics in follow-up.