Conversation
…DO for Expression.HGVS
…if both then these notations needs to say the same thing, then add verbose audit output and configurable strictness for variant checks: - Introduce `--verbose` flag in `parse-excel` to show preprocessing audit steps. - Colorize audit messages (info=cyan, warn=yellow, error=red) and indent them for better readability. - Add `--strict-variants` / `--no-strict-variants` option (default: warn) to treat raw↔HGVS mismatches as errors when enabled. - Pass `strict_variants` through to `DefaultMapper` and update `_check_hgvs_consistency` to honor the new strictness setting.
- **New CLI command** `audit-excel` in `src/P6/__main__.py`
- Options:
- `-e, --excel-path <FILE>` (required) to specify the workbook
- `-r, --report-json` flag to toggle JSON output
- Reads sheets via existing `_read_sheets()`
- Calls `preprocess()` to collect audit entries
- Renders either:
- Human-readable table (columns: SHEET, STEP, LEVEL, MESSAGE; two spaces between columns for reliable parsing)
- JSON array of `{ step, sheet, level, message }` objects with `--report-json`
- **Tests for `audit-excel`** in `tests/test_cli_audit_excel.py`
- `test_audit_excel_table_output`:
- Invokes `audit-excel -e <sample.xlsx>`
- Asserts exit code `0`
- Verifies header line begins with `SHEET`
- Splits each subsequent line on `\s{2,}` and checks for ≥4 columns
- `test_audit_excel_json_output`:
- Invokes with `-r`
- Parses output as JSON list of dicts
- Asserts each dict contains keys `step`, `sheet`, `level`, `message`
- **Unit tests for `preprocess()`** in `tests/test_preprocess.py`
- Fixture `simple_workbook`: generates a minimal valid genotype sheet in a temp Excel file
- `test_preprocess_detects_variant_sheet`:
- Loads tables via `load_sheets_as_tables()`
- Runs `preprocess()`
- Asserts presence of a `classify-sheet` entry for “geno”
- Asserts no error‐level `variant-check` entry on valid input
- **Non-functional changes**
- Imported `json` at top of `__main__.py`
- No modifications to existing `parse-excel` or phenopacket serialization logic
… outputted phenopackets
…preferred design)
…testable components and align interface
- **Align with `TableMapper` interface**
- `apply_mapping` now returns `list[Phenopacket]` (previously returned tuples of record lists)
- Pipeline steps:
1. Select and validate tables via `_choose_named_tables`
2. Map rows to domain records using `_map_*_table` wrappers
3. Group records per patient
4. Build one `Phenopacket` per patient
5. Return the list of packets
- **Explicit table selection**
- Added `KNOWN_SHEET_ALIASES` for common name variants:
- `genotype`: {"genotype", "variants", "variant", "geno"}
- `phenotype`: {"phenotype", "hpo", "pheno"}
- `diseases`: {"disease", "diseases"}
- `measurements`: {"measurement", "measurements", "labs"}
- `biosamples`: {"biosample", "biosamples", "samples"}
- Removed column-based guessing; require explicit/aliased names
- Emit error if both genotype and phenotype sheets are missing
- **Row-level parsers**
- `parse_genotype_row`:
- Splits slash-separated zygosity/inheritance
- Validates against `ZYGOSITY_MAP` / `INHERITANCE_MAP`
- Normalizes email and parses numeric positions
- Returns `(genotype_records, [])`
- `parse_phenotype_row`:
- Extracts HPO ID + optional label
- Normalizes `date_of_observation`
- Uses `_to_bool` for `status` (fixes tuple bug from `bool(row["status"],)`)
- Runs ontology lookups and warns on obsolete/mismatched labels
- Returns `(phenotype_records, term_ids)`
- **Validation utilities**
- `_normalize_time_like`: handles `NaN`, `NaT`, blanks; adds `T` prefix to numeric dates
- `_to_bool`: robust bool parsing from `1/0`, `yes/no`, `true/false`, etc.
- `check_hgvs_consistency`: compares HGVS string to raw coordinate/allele fields; normalizes case and `chr` prefix; warns/errors based on `strict_variants`
- **Sheet-level wrappers**
- `_map_genotype_table`: normalizes index, checks required `GENOTYPE_KEY_COLUMNS`, runs HGVS consistency check, delegates to `_map_genotype`
- `_map_phenotype_table`: checks `PHENOTYPE_KEY_COLUMNS`, delegates to `_map_phenotype` with batch HPO validators
- `_map_diseases_table`, `_map_measurements_table`, `_map_biosamples_table`: normalize index, check required columns, delegate to row mappers
- **Phenopacket assembly**
- `_group_records_by_patient`: aggregates all record types by patient ID
- `construct_phenopacket_for_patient`:
- Sets subject id
- Adds phenotypic features (sets `excluded` if status is false)
- Adds genotype interpretations with HGVS expression and optional location/allele fields
- Adds diseases, measurements, and biosamples minimally
- **Fixes**
- Corrected phenotype `status` parsing to `_to_bool` (prevents always-true bug)
- Centralized column existence checks to fail fast on missing required fields
- **Non-functional**
- Improved docstrings and inline comments for clarity
- Left optional raw/HGVS fallback logic commented for possible future re-enablement
…rve test output
- **DefaultMapper**
- Added `self.stats` to `apply_mapping()` for back-compat record counts
(`genotypes`, `phenotypes`, `diseases`, `measurements`, `biosamples`, `patients`)
- Count values set immediately before returning assembled `Phenopacket`s
- Ensures CLI/tests can still print "Created N Genotype objects" / "Created N Phenotype objects"
without re-grouping data in the CLI
- **CLI (`parse_excel`)**
- Removed legacy Steps 6–9 that rebuilt `records_by_patient` from raw record lists
(variables no longer exist in refactored mapper API)
- Now:
1. Apply mapping → receive full `Phenopacket` list
2. Prepare output dir via `_prepare_output_dir()`
3. Serialize phenopackets directly to numbered `.json` files
4. Print summary:
- "Wrote {patients} phenopacket files to {output_dir}"
- "Created N Genotype objects" / "Created N Phenotype objects" from `mapper.stats`
- Preserves exact output strings and order expected by existing tests
- Fixed syntax error in `out_f.write(MessageToJson(pkt))` (missing `)`)
- **Other**
- Retained warning/error reporting via `_report_issues(notepad)`
- No functional change to mapping logic or file formats
…dding a tiny normalization in `DefaultMapper.parse_genotype_row` before constructing the `Genotype`
- When constructing Phenopackets, strip a leading 'chr' from HGVS g. expressions so output matches canonical form expected by tests (e.g., '16:g.100A>G'). - Keeps validation tolerant of either form; only serialization is normalized.
…S output; cleanup
- Mapper unit tests:
- parse_phenotype_row:
- Happy-path parsing with numeric date normalization.
- Handles NAD entries gracefully, skipping with warning.
- Emits label/name mismatch warnings when ontology term labels differ.
- parse_genotype_row:
- Correctly pairs multi-token zygosity and inheritance fields.
- Errors on invalid or unrecognized zygosity/inheritance codes.
- check_hgvs_consistency:
- Accepts both chr-prefixed and bare chromosome identifiers.
- Supports BED-like SNV formatting.
- In strict mode, errors when genomic positions disagree.
- utils:
- _normalize_time_like correctly handles multiple timestamp formats.
- _to_bool covers a truth table of truthy/falsy string inputs.
- alias selection:
- _choose_named_tables resolves variant, HPO, and laboratory sheet aliases.
- table wrappers:
- Enforces required columns for genotype, phenotype, diseases, measurements, and biosamples.
- apply_mapping (end-to-end):
- Builds a complete Phenopacket for a patient.
- Normalizes IDs (e.g., HP:0000510) and assigns canonical HGVS expression.
- Updates mapper.stats counters correctly for genotypes, phenotypes, and patients.
- CLI/main helper unit tests:
- _prepare_output_dir creates timestamped directories.
- _report_issues outputs WARN and ERROR blocks with clean bullet formatting.
- download command runs with mocked network, verifying correct output.
- mapper: serialize HGVS expressions without the optional 'chr' prefix so output matches
canonical form `16:g.100A>G` expected by tests, while keeping validation tolerant to
either variant.
…ers; canonicalize HGVS output; tidy tests
- mapper:
- Broke down `construct_phenopacket_for_patient` into `_add_phenotypic_features`,
`_add_genotype_interpretations`, `_add_diseases`, `_add_measurements`,
and `_add_biosamples` to reduce cyclomatic complexity (C901) without altering behavior.
- Canonicalize `hgvsg` values by stripping optional 'chr' prefix before serializing into
`VariationDescriptor.expressions[0].value` so output matches expected form
(`16:g.100A>G`) while still accepting either format during parsing.
- tests:
- Removed leftover debug `print` calls in `test_mapper_apply_construct`.
- Removed unused `m` variables in `test_mapper_check_hgvs` flagged by Ruff (F841).
- All 43 tests pass with no Ruff errors.
Result: Cleaner mapper internals, consistent HGVS serialization, and tests free of style violations.
Owner
|
@pnrobinson I've just deleted most of the old branches. I don't see any unique commits here and I noticed that this branch builds off of #6. Do you have any problems fi I first merge |
Collaborator
Author
|
So normally we merge feature branches into develop and thence to main. It is also OK to merge into main, but then we should not have a develop branch. |
Collaborator
Author
|
It is probably also good to close some of the feature branches for clarity! |
Owner
Sorry, that is what I meant to say! It's done |
Closed
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
@VarenyaJ
This branch will allow us to create variant objects using pyphetools.
Let's discuss
Please delete no longer needed branches!