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.
| 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.
|
|
||
| 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?
| # 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) |
| 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.
| 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 re-write this whole docstring to explain what's actually happening now (which isn't obvious). We should definitely include propose all, but also should 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.
| # 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.
| * - 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?
|
|
||
| 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.
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