Repository navigation
feat: add El Salvador and Vietnam persona locales #1136
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
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 |
|---|---|---|
|
|
@@ -399,12 +399,14 @@ class NordColor(Enum): | |
| "en_US": "1.24 GB", | ||
| "en_IN": "2.39 GB", | ||
| "en_SG": "0.30 GB", | ||
| "es_SV": "0.74 GB", | ||
| "fr_FR": "3.87 GB", | ||
| "hi_Deva_IN": "4.14 GB", | ||
| "hi_Latn_IN": "2.7 GB", | ||
| "ja_JP": "1.69 GB", | ||
| "ko_KR": "2.66 GB", | ||
| "pt_BR": "2.33 GB", | ||
| "vi_VN": "2.58 GB", | ||
|
Contributor
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. Please add
Contributor
Author
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. Fixed in 93d735b: both locales are now in the supported-locale list and locale parameter table, and the page also documents their new demographic fields. Internal links across all 90 pages pass validation. Fern check reports zero errors; its only warning is that redirect validation was skipped because the CLI is not authenticated. |
||
| } | ||
|
|
||
| LOCALES_WITH_MANAGED_DATASETS = list[str](NEMOTRON_PERSONAS_DATASET_SIZES.keys()) | ||
|
|
||
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.
Please add the new demographic fields to the sampler's field allowlist:
care_access,disability_status,income_band, andlanguages_spokenfores_SV, andhousehold_sizeforvi_VN. These fields are currently dropped from output, and using them inselect_field_valuesraisesValueError. This differs from existing locales: India'seducation_degree, Japan'sarea, and Brazil'sraceare preserved and filterable, and the UK/Germany addition explicitly extended the allowlist. Please cover the new fields with output-preservation and filtering tests.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.
Thanks for catching this! Fixed in 93d735b: added all four es_SV fields and vi_VN household_size to the demographic allowlist. Added regression tests using local Parquet fixtures and the real reader/sampler to verify filtering and output preservation for each field, both with and without synthetic personas. All 160 focused tests and Ruff checks pass. Sex-label normalization remains tracked separately in #1138.