feat: Generalize metrics and add nullability delta metric#43
feat: Generalize metrics and add nullability delta metric#43Moritz Potthoff (MoritzPotthoffQC) wants to merge 22 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 14 +3
Lines 1018 1052 +34
=========================================
+ Hits 1018 1052 +34 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
EgeKaraismailogluQC
left a comment
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?
Co-authored-by: EgeKaraismailogluQC <128645043+EgeKaraismailogluQC@users.noreply.github.com>
…_fraction.py Co-authored-by: EgeKaraismailogluQC <128645043+EgeKaraismailogluQC@users.noreply.github.com>
| def null_fraction_change(left: pl.Expr, right: pl.Expr) -> pl.Expr: | ||
| """Change in the fraction of null entries, rendered as ``<old> -> <new> (<delta>)``. | ||
|
|
||
| ``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). This metric function can be applied to columns | ||
| of any type. | ||
| """ |
There was a problem hiding this comment.
(moving this here to get a thread)
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
I see, good point!
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?
Right now, I am really only interested in the variant implemented here right now.
As I see it, we currently follow two objectives:
- The existing metrics are metrics about the change between numeric columns. I.e., we try to describe the change itself.
- My new metric describes the old and new datasets individually, so that you can understand how a change affects the data.
I think (1) makes sense for numeric columns and in the context of diffly. (2) is also nice since it allows us to understand how the overall data changed (and it generalizes to non-numeric columns).
I could imagine that it would even be nice to expand on (2) in the future (e.g., by providing more metrics in this way). If we do that, we should probably make the differentiation obvious (e.g., through different modules). Wdyt, EgeKaraismailogluQC ?
There was a problem hiding this comment.
If we do that, we should probably make the differentiation obvious (e.g., through different modules)
My impression is that we should probably even do this now already
There was a problem hiding this comment.
Yes, I tend to agree after some more thought. How should we name these two kinds of metrics? I thought of "change metrics" for (1) and "data metrics" for (2). Do you have any other suggestions? Would it make sense to expose two name spaces, i.e. metrics.change and metrics.dataset, or do you have another idea?
There was a problem hiding this comment.
I'd go for metrics.data, but sounds good otherwise. I will extend this PR accordingly
There was a problem hiding this comment.
Implemented this. PTAL again
There was a problem hiding this comment.
Makes sense! Pinging Marius Merkle (@MariusMerkleQC) and Oliver Borchert (@borchero) here since we already discussed these two types of metrics before. The implementation looks sound to me, and the docs are clear. Thanks!
There was a problem hiding this comment.
I think the distinction between these two types of metrics makes a lot of sense. However, I currently find the summary report a little confusing. In my mind, the column-level section only describes the change and it feels a little strange to have dataset-level information in there 🤔
There was a problem hiding this comment.
I think the different layout (old -> new (delta) for the data metrics vs scalar for the change metrics) helps, but I agree that the difference is subtle and it is easy to miss what we compute here (especially with the current metric names). I think to address this, we could keep all the change metrics where they currently are (along with the match rate) and add a new section that contains the new data metrics for each column.
For naming, I propose Data Metrics to make clear that this section does not directly describe the change anymore (as above), but actually covers the data.
On an implementation level, I think this would require significantly more changes than we currently have (especially to have a clean interface to specify both of these types).
Do you see any other alternatives?
There was a problem hiding this comment.
I think the "match rate" (which does not include the ->) is the obvious example where this argument breaks 😅
That being said, I would be fine to keep it like it is as a temporary "workaround". After all, it only affects display, not the package's API.
Going forward, we could (1) either have a new section (as you described) or (2) somehow partition columns to not repeat columns. I think (1) makes a lot more sense. Personally, I would probably go with a naming along the lines of "inspection" but don't have a strong preference for anything.
There was a problem hiding this comment.
I think the "match rate" (which does not include the ->) is the obvious example where this argument breaks 😅
I guess one could see the match rate as a metric of the change ((left == right).mean()) 😂
That being said, I would be fine to keep it like it is as a temporary "workaround". After all, it only affects display, not the package's API.
Okay 👍 Then I will remove the new data metric from the default metrics for now so that we don't display them side-by-side by default for now.
I think (1) makes a lot more sense. Personally, I would probably go with a naming along the lines of "inspection" but don't have a strong preference for anything.
It has the additional advantage that we have more horizontal space to work with 😅
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 43 out of 43 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
diffly/metrics/change.py:73
- Renaming
DEFAULT_METRICStoDEFAULT_CHANGE_METRICSindiffly.metrics.changeis a backwards-incompatible API change for consumers importing the old name. Consider keepingDEFAULT_METRICSas an alias to avoid breaking downstream code.
|
Waiting for this discussion to be settled before merging |
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 and provide metrics that describe the two datasets, not only the change. The specific use-case I have for this is to explain how the share of null values in a column changed.
Changes
metricssubpackagechangesubmoduledatasubmodule for the new metric type (not only numeric)<old> -> <new> (<delta>).