Repository navigation
Conversation
There was a problem hiding this comment.
Wow! This is incredible! I was blown away by its ability to choose generators for the Artist table for the MOMA database! Of course it had trouble with the Artwork database, choosing username for lots of things but that's probably mostly down to us not having URL or ID Code generators.
The documentation is also fantastic.
I liked the table having colours, but we really need them coloured by score band or something (perhaps just highlighting the recommendation). I really liked seeing the recommendation highlighted when it was in Front 2 or 3, so it was weird that actually this was supposed to be a warning highlight!
This does make propose even slower for multiple columns; perhaps we need to do some work to figure out how bad it is, because it might be that creating the proposers is 90% of it and so it's not a huge deal.
Most of the comments I made aren't a big deal and we shouldn't add months to this because we need to get it into the users' hands!
In general, it looks like you have given too much respect to my existing code and functionality. I'm sure you can do better!
Changes I would like to see before approving this:
- Fix the
help proposetext (which is thedo_proposedocstring). No need yet to add extra help topics if you don't want to, though I would like to see that. - Highlight the recommendation.
- Remove the old output for
propose. - At least think about taking out the special casing of the float generators being proposed for int columns. Whether we permit floats in int for now or do the actual fix.
_get_proposer_proposalsmust not store theinclude_allvalue unless I'm completely wrong about this. I think fixing this requires not filtering out the special cased float generators -- whichever way we go on that.
|
|
||
| def analyse_column(values: list[Any]) -> ColumnStats: | ||
| """Compute length/space/digit/punctuation statistics over ``values``.""" | ||
| values = [str(v) for v in values if v is not None] |
There was a problem hiding this comment.
This feels odd. Is the cast to str so that we can take in numbers, dates etc.? What value do we gain from this? I'm imagining floating point ("these have an average of 1 '.' and no spaces"). Is it for safety? This might be OK.
There was a problem hiding this comment.
Yes — it's for safety/generality, though not specifically for numbers/dates: ColumnEvaluator.setup() already routes Numeric/Integer to choose_numeric_profile and Date/DateTime/Time straight to TEMPORAL before anything reaches analyse_column, so your "average of 1 '.' and no spaces" case can't actually occur via that path today.
What does land here is whatever else a non-numeric, non-temporal column type hands back that isn't already a plain str — Boolean, UUID, Enum, JSON, bytes, etc. — which would blow up on len(v)/v.count(" ") without the cast. There's a test pinning this down (test_non_string_values_are_stringified).
I've added a short comment here pointing at the setup() dispatch so the next reader doesn't have to trace it the way I just did.
| return EvaluationProfile.SHORT_TEXT | ||
|
|
||
| # repeated values | ||
| if stats.uniqueness < CATEGORICAL_UNIQUENESS_THRESHOLD: |
| repeated categories (a status code, rating, or flag) or like a | ||
| continuous / high-cardinality quantity (an age, salary, or identifier). | ||
|
|
||
| Previously every Integer column was assumed to be categorical and every |
There was a problem hiding this comment.
This is great information for me, but probably not for future readers of the code as they are not thinking about how the code used to be.
There was a problem hiding this comment.
Fair - dropped the "previously..." framing.
|
|
||
| with engine.connect() as conn: | ||
| if len(columns) == 1: | ||
| # Cap how many real rows we pull into memory so this scales to |
There was a problem hiding this comment.
So you mean we must do ORDER BY RANDOM before LIMIT? Well yes. This is done elsewhere in DataFaker without comment. If you feel this does need a comment, should we instead move it into dialects.py and call it maybe RandomSample(sample_size)? Then we could put this comment in there and make each ORDER BY RANDOM LIMIT explicitly a random sample operation? What do you think?
There was a problem hiding this comment.
Agreed on the idea, but it's bigger than this PR - I count ~13 call sites across 11 files (proposers/, evaluators/, interactive/), and one of them (extract.py) doesn't even use .limit(), it does .fetchmany() on the cursor instead, so it'd need an actual rewrite rather than a mechanical rename. I'd rather not scope-creep this PR with an 11-file refactor for something that's pre-existing convention, not new here.
Opened #143 to track it: a random_sample(stmt, sample_size) helper in dialects.py (probably a plain function rather than a ColumnElement like Random/StdDev, since it wraps a statement, not a column), with the comment living there once.
| # reproducing them verbatim just leaks real records). | ||
| non_null_real_values = [v for v in self.real_values if v is not None] | ||
| self.real_uniqueness = ( | ||
| len({str(v) for v in non_null_real_values}) / len(non_null_real_values) |
There was a problem hiding this comment.
Same philosophy as the analyse_column cast, but even more necessary here: real_uniqueness is computed unconditionally for every column type (before any Numeric/Date routing happens), so it needs to work on whatever a driver hands back - and it doubles as the canonical representation used to compare real vs. synthetic values elsewhere in this file (see evaluate()'s copy_fraction, which does the same str(canonicalize_value(v)) pattern) - e.g. so an int real value and a str synthetic value that represent the same thing are correctly recognised as a match.
| f"{penalty_multipliers[idx]:.3f}", | ||
| ] | ||
| # color entire row for Pareto front 1 | ||
| if front == 1 and theme is not None: |
There was a problem hiding this comment.
With my experimentation, most of the table is purple most of the time. Sometime it's all purple, sometimes the line you want is in yellow. I don't think people care that much about the Pareto front (or even what that means), but they might well want the recommended line highlighted. So, I'd prefer setting the recommendation to one theme element and everything else to another theme element; or even put the recommendation in one colour, the near misses in another and the rest in yet another.
There was a problem hiding this comment.
Added two dedicated theme elements - recommend and near_miss - rather than reusing column/function for this. The recommended row now gets recommend (bright green in both DARK/LIGHT), other Pareto front 1 rows ("near misses" - non-dominated alternatives) get near_miss (blue/cyan), and everything else is left uncoloured, so it's the recommendation vs. near-misses vs. the rest as three distinct, clearly separated colours rather than one colour covering most of the table.
| They will be listed in order of fit, the most likely matches first. | ||
| The results can be compared (against a sample of the real data in | ||
| the column and against each other) with the 'compare' command. | ||
|
|
There was a problem hiding this comment.
This docstring appears in the response to help propose or ?propose. This paragraph is pretty confusing in this help. We need to re-write this whole docstring to explain what's actually happening now (which isn't obvious). We should definitely include propose all, but also could say the user can type help ranking or help pareto for help on those topics; to allow this we would need to override do_help.
There was a problem hiding this comment.
Rewrote it to explain the current behavior plainly: what the scores mean, that the single best one is highlighted as the recommendation, and the two specific things left out by default (dominated candidates beyond the cap, and type-incompatible ones) with 'propose all' as the escape hatch for both - trying to avoid requiring the reader to already know what "Pareto front" means to follow it.
help ranking/help pareto sub-topics done. Also added one line pointing at the "Evaluating and ranking proposals" doc section (by name, not a Sphinx :ref: - this text prints as plain terminal output via help propose, so cross-ref markup would just show up literally) for anyone who wants the full detail.
| # which is exactly why the default 'propose' excludes it structurally | ||
| # instead of trying to penalize it. | ||
| recommended_proposer = results[ranking.recommended_index].proposer | ||
| if self._is_integer_incompatible(recommended_proposer, columns): |
There was a problem hiding this comment.
Let's get rid of this; it isn't for the user to be scared of proposers that might not work. We should make them work or wait for the issue report.
There was a problem hiding this comment.
Removed, along with the type-compatibility check it was guarding - see the generators.py:839 thread for where that logic ended up instead (a narrower, profile-scoped guard that excludes the candidate from the recommendation entirely rather than warning about it).
| * - A near-unique value that must look real but not *be* real, e.g. an | ||
| email address or free-text ID | ||
| - Whichever ``generic.*`` value proposer ``propose`` recommends | ||
| - ``propose`` applies an extra privacy penalty here: a candidate that |
There was a problem hiding this comment.
Is "privacy penalty" a known term (can't find it through searching). This sentence reads as though privacy is worsened through this technique. "privacy measure" perhaps?
There was a problem hiding this comment.
Fair, couldn't find it as a standard term either - changed to "privacy safeguard".
|
|
||
| def test_avg_length_and_ratios(self) -> None: | ||
| """avg_length, space_ratio, digit_ratio and punctuation_ratio are computed.""" | ||
| stats = analyse_column(["ab 1!"]) |
There was a problem hiding this comment.
Maybe make the three stats that are tested for different numbers, otherwise you aren't testing that they are written to the correct attributes.
There was a problem hiding this comment.
You are right, all three ratios happened to be 1/5 in that example, so a bug that swapped e.g. digit_ratio and space_ratio would still have passed. Changed to "a 1!!!" (2 spaces, 1 digit, 3 punctuation out of 7 chars) so each assertion actually pins down its own attribute.
|
Hi @tim-band Thanks for such a thorough first pass! This pushed the PR somewhere much better than where I left it. Went through every thread. Quick roundup, then the things I’d still like your explicit input on before calling this settled. Addressed (replied individually on each thread, diffs speak for themselves): Still want your input on:
Re-requesting review once you’ve had a look. |
Responds to tim-band's review on PR #137: - Scope the float-for-Integer type guard to IDENTIFIER-profile columns only (CATEGORICAL columns already self-correct on fidelity), computed per 'propose' call instead of cached in _get_proposer_proposals - Give PK/UNIQUE columns precedence to proposers that structurally guarantee fresh values (new Proposer.guarantees_fresh_uniqueness(), set on IncrementProposer) over the generic fidelity score, which can never credit a sequence proposer for deliberately exceeding the observed range - Remove the old unranked per-proposer 'propose' output and the include_all caching it required; highlight the recommended row (not just Pareto front 1) with dedicated theme colours (recommend/near_miss) that remain visible with theming off - Fold ColumnEvaluator.setup() into its constructor and consolidate is_numeric_sql_type into db_utils.py, shared with partitioned.py - Fix WeekdayExtractor: its SQL and Python sides used disagreeing day-of-week numbering conventions, silently comparing mismatched category labels for every temporal column; replaced with portable floor/modulo arithmetic on the existing days-since-epoch value - Fix VowelConsonantPatternExtractor miscounting accented vowels as consonants (Unicode NFKD base-letter decomposition) - Rename EvaluationPipeline to EvaluationCriterion (and PIPELINE(S) to CRITERION/CRITERIA throughout): these are independent, weighted measures combined by weighted average, not a sequential pipeline - Add 'help ranking'/'help pareto' topics; rewrite the 'propose' docstring to match current behaviour - Assorted docstring/type-hint/wording fixes and test clarity fixes raised in review Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Only one issue; the Theme cannot be null, so we can simplify there.
Great job!
Oh hold on, I didn't answer all your questions. I'll do that tomorrow.
Answers:
- Your solution seems good. I would like to talk to you about what exact problems you were seeing though.
- Your extra theme entries seem good.
- OK, thanks for the research!
So, just the Theme thing.
| cached. | ||
| """ | ||
| if include_all is None: | ||
| include_all = self.proposers_include_all |
There was a problem hiding this comment.
Ah, no I see why it might be correct. I still don't really like it; this extra state makes things much harder to reason about and test correctly.
| results: Sequence[ProposalEvaluation], | ||
| profile: EvaluationProfile | None, | ||
| theme: Any | None = None, | ||
| theme: Theme | None = None, |
There was a problem hiding this comment.
get_active_theme cannot return None, so we could remove the | None and all the resulting ifs
This branch has not been deployed
2 participants
Resolved #112
Summary
Adds a column-level evaluation layer that scores and ranks candidate generators against real data, and uses it to improve the
proposecommand inconfigure-generators.datafaker.evaluatorspackage:ColumnEvaluatorsamples a real column, picks an evaluation profile (short text / free text / categorical / identifier) from its statistics, and scores each candidate proposer's synthetic output against it using novelty, diversity, and statistical fidelity metrics (metrics.py,statistical_fidelity.py,feature_extractors.py,distribution_builders.py).proposal_ranking.py: normalises the heterogeneous raw fit scores into a probabilistic ranking and truncates the displayed list with entropy-based cutoff, soproposeno longer dumps every candidate unranked.proposecommand (interactive/generators.py): now surfaces ranked proposals (capped atMAX_PROPOSERS_SHOWN = 10,propose allstill shows everything) and filters out continuous-float proposers for genuinelyInteger-typed columns, since inserting a non-whole float into an Integer column is backend-dependent.IncrementProposer(proposers/sequence.py): a new proposer for integer primary keys that continues the real data's max value, reusing the samegeneric.column_value_provider.incrementmechanismmake.pyalready uses as its default for integer PKs. (Works towards Add a UniqueSequenceProposer for unique/ID columns — no current proposer guarantees non-duplicate values #134)dialects.py: addsWordCount/SentenceCount(and related) SQL expression helpers, with Postgres/DuckDB and MSSQL compilation, used by the new feature extractors.builtin_generators.rstandchoosing_a_generator.rst, glossary entries for Generator/Proposer/Role, and TOC updates. (Work towards https://github.com/alan-turing-institute/DataMatryoshka/issues/14)Implements the "Enhancement to generators selection layer" half of #112 (keyword/ranking improvements to
propose, without changing generator/fit logic) plus the groundwork column-level fidelity metrics for the evaluation layer described there. Theevaluatecommand and table/dataset-level utility & privacy metrics from #112 are not part of this PR -- a new issue will be created for this.Further details on the generator evaluation and ranking methods can be found here: https://safehr-data.github.io/datafaker/builtin_generators.html#evaluating-and-ranking-proposals
Note
@tim-band your original fit-value table is left in the codebase alongside the new ranked table — I didn't want to remove it without your sign-off. Specifically, this PR does not touch:
interactive/generators.py:1072-1090— the old per-proposer loop that printsPROPOSE_GENERATOR_SAMPLE_TEXTusingprop.fit(-1)Proposer.fitinproposers/base.py:140, which that loop callsOnce you've reviewed and are happy with the ranking table (
RANKED_SAMPLE_TEXT) replacing this output, let me know and I'll remove both in a follow-up — or if you'd rather keep both tables shown side by side permanently, let me know that too and I'll leave them as-is.Test plan
poetry run pytest tests/test_evaluators_*.py— 169 passedconfigure-generators→proposeinteractively against a real table to confirm ranked output andpropose allstill work as expected