Repository navigation
Add synthetic corpus generator for two example cohorts - #25
Conversation
✅ Deploy Preview for study-palette canceled.
|
There was a problem hiding this comment.
Pull request overview
Adds a deterministic synthetic-corpus generator and BDCHM-targeted dm-bip transformation specs for two fictional example cohorts, enabling portal development/testing without using participant data while remaining structurally faithful to harmonized BDC/BDCHM outputs.
Changes:
- Introduces a deterministic population model and raw dbGaP-style table emitter for two example studies.
- Generates and commits BDCHM-targeted mapping specs for both studies, plus a validation script with distribution/invariant checks.
- Adds CI workflow to regenerate → generate specs → validate → YAML-parse on
synthetic/**changes, and expands ruff’s lint include to coversynthetic/**.
Reviewed changes
Copilot reviewed 42 out of 42 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| synthetic/vocab.py | Centralizes coded CURIE/enum values used by synthetic data/specs |
| synthetic/population.py | Deterministic cohort/population model and measurement generation |
| synthetic/generate.py | Emits dbGaP-style raw tables from the population model |
| synthetic/specs.py | Programmatically generates dm-bip transformation specs for BDCHM |
| synthetic/validate.py | Validates corpus distributions/invariants across studies |
| synthetic/README.md | Documents goals, usage, and structural features exercised |
| synthetic/fetch-bdchm.sh | Fetches pinned upstream BDCHM schema YAML |
| synthetic/.gitignore | Ignores generated corpus/output and fetched schema |
| synthetic/pipeline/example_study_one.mk | dm-bip pipeline config for Example Study One |
| synthetic/pipeline/example_study_two.mk | dm-bip pipeline config for Example Study Two |
| synthetic/specs/example_study_one/person_participant.yaml | Person/Participant mapping for Study One |
| synthetic/specs/example_study_one/demography.yaml | Demography mapping for Study One |
| synthetic/specs/example_study_one/visit.yaml | Visit mapping for Study One |
| synthetic/specs/example_study_one/blood_pressure.yaml | BP observation-set mapping for Study One |
| synthetic/specs/example_study_one/height.yaml | Height measurement mapping for Study One |
| synthetic/specs/example_study_one/weight.yaml | Weight measurement mapping for Study One |
| synthetic/specs/example_study_one/bmi.yaml | BMI measurement mapping for Study One |
| synthetic/specs/example_study_one/hdl.yaml | HDL measurement + assay/limits mapping for Study One |
| synthetic/specs/example_study_one/bun.yaml | BUN measurement (with qualifier) mapping for Study One |
| synthetic/specs/example_study_one/wbc.yaml | WBC measurement + assay mapping for Study One |
| synthetic/specs/example_study_one/cond_heart_failure.yaml | Heart failure condition mapping for Study One |
| synthetic/specs/example_study_one/cond_family_stroke.yaml | Family stroke condition mapping for Study One |
| synthetic/specs/example_study_one/cond_hypertension.yaml | Hypertension condition mapping for Study One |
| synthetic/specs/example_study_one/cond_heart_attack.yaml | Heart attack condition mapping for Study One |
| synthetic/specs/example_study_one/drug_exposure.yaml | Drug exposure mapping for Study One |
| synthetic/specs/example_study_two/person_participant.yaml | Person/Participant mapping for Study Two |
| synthetic/specs/example_study_two/demography.yaml | Demography mapping for Study Two |
| synthetic/specs/example_study_two/visit.yaml | Visit mapping for Study Two |
| synthetic/specs/example_study_two/blood_pressure.yaml | BP observation-set mapping for Study Two |
| synthetic/specs/example_study_two/height.yaml | Height measurement mapping for Study Two |
| synthetic/specs/example_study_two/weight.yaml | Weight measurement mapping for Study Two |
| synthetic/specs/example_study_two/bmi.yaml | BMI measurement mapping for Study Two (categorical) |
| synthetic/specs/example_study_two/hdl.yaml | HDL measurement + assay/limits mapping for Study Two |
| synthetic/specs/example_study_two/bun.yaml | BUN measurement (with qualifier) mapping for Study Two |
| synthetic/specs/example_study_two/wbc.yaml | WBC measurement + assay mapping for Study Two |
| synthetic/specs/example_study_two/cond_heart_failure.yaml | Heart failure condition mapping for Study Two |
| synthetic/specs/example_study_two/cond_family_stroke.yaml | Family stroke condition mapping for Study Two |
| synthetic/specs/example_study_two/cond_hypertension.yaml | Hypertension condition mapping for Study Two |
| synthetic/specs/example_study_two/cond_heart_attack.yaml | Heart attack condition mapping for Study Two |
| synthetic/specs/example_study_two/drug_exposure.yaml | Drug exposure mapping for Study Two |
| pyproject.toml | Expands ruff include scope to lint synthetic Python code |
| .github/workflows/test-synthetic.yaml | CI job to generate/spec/validate and parse YAML on synthetic changes |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 42 out of 42 changed files in this pull request and generated no new comments.
Suppressed comments (5)
synthetic/specs.py:183
- In blood_pressure() the generated YAML sets unit to "mm[Hg]"" (a trailing quote character). This will propagate into the generated specs and makes the unit value incorrect even though YAML still parses.
value_decimal:
populated_from: {source}
unit:
value: "mm[Hg]\""""
synthetic/population.py:176
- telehealth_at is chosen from range(study.visits) before potentially truncating visits for deceased participants; if count < telehealth_at+1, a deceased participant will have 0 TELEHEALTH visits, contradicting the stated invariant of exactly one TELEHEALTH visit per participant.
telehealth_at = rng.randrange(study.visits)
count = study.visits
if person.deceased:
count = rng.randint(1, study.visits)
synthetic/validate.py:9
- The docstring suggests running
python synthetic/validate.py, but this file uses local imports (e.g.,import population) and there is nosynthetic/__init__.py, so running from the repo root will fail. The invocation should match the actual supported usage (run from the synthetic directory) or the package structure/imports should be updated.
synthetic/specs.py:12 - The docstring suggests running
python synthetic/specs.py, but this module uses local imports (import generate,import population) andsyntheticis not a package, so that invocation from repo root will fail. Update the invocation text to the supported pattern (run from the synthetic directory) or convert synthetic into a package.
Patterned on RTI's priority_variables_transform specs: the same uuid5 identity
scheme, the same slot names, the same coded values.
python synthetic/specs.py [--out DIR]
"""
synthetic/generate.py:11
- The docstring suggests running
python synthetic/generate.py, but this script relies on local imports (import population) andsyntheticis not a package, so that invocation from repo root will fail. Update the invocation text to match how CI/README run it (from the synthetic directory) or package-ify synthetic.
identical to real harmonized data, rather than a second implementation of the
transformation that can quietly drift.
python synthetic/generate.py [--out DIR]
"""
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 51 out of 51 changed files in this pull request and generated no new comments.
Suppressed comments (4)
synthetic/population.py:176
- telehealth_at is chosen from study.visits even when deceased participants have their visit count truncated. When count < telehealth_at+1, the participant ends up with 0 TELEHEALTH visits, contradicting the docstring/README guarantee of exactly one TELEHEALTH visit per participant.
telehealth_at = rng.randrange(study.visits)
count = study.visits
if person.deceased:
count = rng.randint(1, study.visits)
synthetic/validate.py:74
- The validator only checks for participants with >1 TELEHEALTH visit, but it doesn't fail if a participant has 0 TELEHEALTH visits. Given the intended invariant (“one TELEHEALTH visit per participant”), this should be enforced as exactly one (!= 1) so regressions are caught in CI.
synthetic/README.md:87 - This README claims the corpus exercises
associated_evidenceon study-record-sourced conditions, but no generator/spec/sample code in synthetic/ emits or demonstrates that field (the only occurrence is this bullet). Either implement the field or mark it as not yet emitted to avoid misleading consumers.
- `associated_evidence` on study-record-sourced conditions
synthetic/specs.py:399
- drug_exposure() hard-codes the exposure_provenance permissible value even though vocab.py defines SELF_REPORTED_MEDICATION. Using the shared constant keeps the generator/specs aligned if the enum spelling ever changes.
exposure_provenance:
value: PATIENT_SELF_REPORTED_MEDICATION
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 51 out of 51 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
synthetic/specs.py:399
drug_exposure()hard-codesPATIENT_SELF_REPORTED_MEDICATIONeven though the same permissible value is already centralized invocab.py(v.SELF_REPORTED_MEDICATION). Using the shared constant avoids drift if the enum value ever changes.
populated_from: {phv(study, "meds", "CCB_STATUS")}
exposure_provenance:
value: PATIENT_SELF_REPORTED_MEDICATION
"""
synthetic/population.py:176
telehealth_atis chosen fromrange(study.visits)before deceased participants have their visit count truncated. Whencount < study.visits, this can produce participants with zero TELEHEALTH visits (iftelehealth_at >= count), contradicting the documented invariant of one telehealth visit per participant.
telehealth_at = rng.randrange(study.visits)
count = study.visits
if person.deceased:
count = rng.randint(1, study.visits)
visits = []
for i in range(count):
category = v.TELEHEALTH if i == telehealth_at else v.STUDY_SITE_VISIT
synthetic/validate.py:74
- The visit-structure validation only checks for participants with >1 TELEHEALTH visit, but it doesn't fail if a participant has 0 TELEHEALTH visits. Given the brief/invariants around exactly one TELEHEALTH visit per participant, the validator should assert both bounds so regressions are caught early.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 51 out of 51 changed files in this pull request and generated no new comments.
Suppressed comments (6)
synthetic/specs.py:450
- main() calls build(study) twice (once to write files and again just to compute len()), duplicating work and risking inconsistencies if build() ever becomes non-trivial or non-deterministic. Build once and reuse the dict for both writing and reporting.
for name, body in build(study).items():
(out / f"{name}.yaml").write_text(body)
print(f"{study.name}: {len(build(study))} specs -> {out}")
synthetic/population.py:175
- In _make_visits(), telehealth_at is chosen before truncating the visit count for deceased participants, so some participants can end up with 0 TELEHEALTH visits (when telehealth_at >= count), contradicting the function docstring and README claim of exactly one TELEHEALTH visit per participant.
telehealth_at = rng.randrange(study.visits)
count = study.visits
if person.deceased:
count = rng.randint(1, study.visits)
synthetic/validate.py:74
- The telehealth validation only checks for participants with >1 TELEHEALTH visit; it won’t fail if a participant has 0 TELEHEALTH visits (which can happen if generation changes). If the invariant is “exactly one”, validate that directly to prevent silent drift.
synthetic/validate.py:77 - This comment claims “the deceased attend fewer visits than the living”, but the code below only checks visit-age monotonicity and doesn’t enforce visit-count differences. Consider updating the comment to match what’s actually validated.
synthetic/specs.py:398 - drug_exposure() hard-codes the exposure_provenance value instead of reusing the constant in vocab.py. Using v.SELF_REPORTED_MEDICATION keeps the generator consistent with the project’s centralized coded-value definitions and reduces the chance of accidental divergence.
This issue also appears on line 448 of the same file.
value: PATIENT_SELF_REPORTED_MEDICATION
.github/workflows/test-synthetic.yaml:55
- The inline YAML-parse check opens each spec file without closing it. Use a context manager to avoid leaking file descriptors (especially if the spec count grows).
for path in paths:
try:
yaml.safe_load(open(path))
except yaml.YAMLError as exc:
There was a problem hiding this comment.
🟡 Changes recommended
Several synthetic “study-record”/provenance claims are not yet reflected in emitted specs/output (and the README currently documents a feature that isn’t produced).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (8)
Previously missed (6) — in code that hasn't changed since the last review.
synthetic/generate.py:120
HA_SOURCEwritesSELF_REPORT, but this value is not defined in synthetic/vocab.py and doesn't match the BDCHM/RTI-style provenance string already used elsewhere (PATIENT_SELF-REPORTED_CONDITION). If/when this column is mapped into harmonized output,SELF_REPORTwill become a vocabulary drift point.
synthetic/specs.py:361condition()currently only supports a fixedcondition_provenancevalue, which makes it hard to reflect per-row provenance (e.g., heart attacks sourced fromHA_SOURCE). Introduce an optionalprovenance_coland precompute the appropriately-indented provenance line for the YAML template.
This issue also appears on line 373 of the same file.
synthetic/specs.py:432
HA_SOURCEis generated in the rawconditionstable but never used by the transformation specs. If the intent is to distinguish study-record vs self-reported heart attacks, passprovenance_col="HA_SOURCE"for the heart-attack Condition so the emittedcondition_provenancecan vary per participant.
synthetic/README.md:127- The README claims the corpus exercises
associated_evidenceon study-record-sourced conditions, but there is noassociated_evidencefield emitted anywhere in the repo (only this mention). This is misleading for consumers/reviewers; either implement the feature or adjust the README to match what is actually produced.
synthetic/specs.py:399 drug_exposure()hard-codesPATIENT_SELF_REPORTED_MEDICATIONinstead of using the canonical constant invocab.py(v.SELF_REPORTED_MEDICATION). Using the constant avoids accidental drift if the permissible value ever changes or needs to be referenced elsewhere.
synthetic/specs.py:450main()recomputesbuild(study)just to print the spec count, which is unnecessary work and can diverge ifbuild()ever becomes non-deterministic. Cache the dict once per study and reuse it for writing and reporting.
synthetic/specs.py:377
- The YAML template still hard-codes
condition_provenanceas a fixedvalue: .... After addingprovenance_line, the template should emit that line so callers can choose between a fixed value andpopulated_froma column.
populated_from: {phv(study, "conditions", status_col)}
condition_provenance:
value: {provenance}
relationship_to_participant:
{relationship_line}
synthetic/schema.py:157
spec_shape()loads YAML viayaml.safe_load(open(path)), which also leaves file handles unmanaged. Wrapping the read inwith open(...)avoids descriptor leaks and is consistent with the rest of the file’s IO style.
merged = {}
for path in sorted(Path(spec_dir).glob("*.yaml")):
for doc in yaml.safe_load(open(path)) or []:
for cls, slots in walk(doc.get("class_derivations")).items():
merged.setdefault(cls, {}).update(slots)
- Files reviewed: 52/52 changed files
- Comments generated: 1
- Review effort level: Lite
| def __init__(self, path): | ||
| """Load and index the model.""" | ||
| schema = yaml.safe_load(open(path)) | ||
| self.classes = schema.get("classes", {}) | ||
| self.slots = schema.get("slots", {}) |
Two fictional cohorts, structurally faithful to harmonized BDC data, for teams building against the portal without touching participant data.
The generator emits dbGaP-style raw tables, which dm-bip transforms against BDCHM. Emitting BDCHM directly would have been a second implementation of the transformation, free to drift from the real one until a portal built on it met real data.
Every coded value is attested in RTI's
priority_variables_transformspecs or in BDCHM — the MONDO/HP concepts, the OMOP demography and relationship codes, all four calcium channel blocker CURIEs. The two exceptions are the MMO assay methods, which appear nowhere in the real specs and were looked up in MMO rather than invented.Both studies run clean through the pipeline: exit 0, no unsatisfied required slots. 500 participants each, with 25 individuals enrolled in both — sharing a
dbGaP_Subject_IDso harmonization resolves them to onePersonwith twoParticipantrecords, as real cross-study participation does.The corpus itself is not committed. Generation is deterministic given a seed, so CI regenerates and validates rather than diffing against something stored; built corpora are distributed as release assets.
validate.pycarries 50 checks — distributions with tolerance, invariants exact. New workflow runs generate → specs → validate → YAML parse, path-filtered tosynthetic/**.Two things worth a reviewer's attention: ruff's
includewas scoped toapi/**, so this addssynthetic/**and widens what CI lints. And BDCHM is fetched pinned at v1.3.0 rather than vendored, so an upstream schema change is a deliberate bump.Coverage against BDCHM's full slot inventory — the acceptance number for broadening past the brief — is not built yet. This pass covers the brief only.