Update score log structure #49
Merged
Conversation
This file contains 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
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## fix-nonadditive #49 +/- ##
===================================================
+ Coverage 89.18% 89.95% +0.77%
===================================================
Files 40 39 -1
Lines 2320 2508 +188
===================================================
+ Hits 2069 2256 +187
- Misses 251 252 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
nebfield
commented
Sep 13, 2024
Comment on lines
-29
to
-153
| "pgs_id", | ||
| "pgp_id", | ||
| "pgs_name", | ||
| "genome_build", | ||
| "variants_number", | ||
| "trait_reported", | ||
| "trait_efo", | ||
| "trait_mapped", | ||
| "weight_type", | ||
| "citation", | ||
| "HmPOS_build", | ||
| "HmPOS_date", | ||
| "format_version", | ||
| "license", | ||
| ) | ||
|
|
||
| _default_license_text = ( | ||
| "PGS obtained from the Catalog should be cited appropriately, and " | ||
| "used in accordance with any licensing restrictions set by the authors. See " | ||
| "EBI " | ||
| "Terms of Use (https://www.ebi.ac.uk/about/terms-of-use/) for additional " | ||
| "details." | ||
| ) | ||
|
|
||
| def __init__( | ||
| self, | ||
| *, | ||
| pgs_name, | ||
| genome_build, | ||
| pgs_id=None, | ||
| pgp_id=None, | ||
| variants_number=None, | ||
| trait_reported=None, | ||
| trait_efo=None, | ||
| trait_mapped=None, | ||
| weight_type=None, | ||
| citation=None, | ||
| HmPOS_build=None, | ||
| HmPOS_date=None, | ||
| format_version=None, | ||
| license=None, | ||
| ): | ||
| """kwargs are forced because this is a complicated init and from_path() is | ||
| almost always the correct thing to do. | ||
|
|
||
| Mandatory/optional fields in a header are less clear than columns. The | ||
| Catalog provides this information automatically but scoring files from other | ||
| places might not. | ||
|
|
||
| We don't want to annoy people and force them to reformat their custom scoring | ||
| files, but we do require some minimum information for the calculator, | ||
| so pgs_name and genome_build are mandatory. | ||
| """ | ||
| self.pgs_name = pgs_name | ||
| self.genome_build = GenomeBuild.from_string(genome_build) | ||
|
|
||
| if self.genome_build is None: | ||
| # try overwriting with harmonised data | ||
| self.genome_build = GenomeBuild.from_string(HmPOS_build) | ||
|
|
||
| if self.pgs_name is None: | ||
| raise ValueError("pgs_name cannot be None") | ||
|
|
||
| # the rest of these fields are optional | ||
| self.pgs_id = pgs_id | ||
| self.pgp_id = pgp_id | ||
|
|
||
| try: | ||
| self.variants_number = int(variants_number) | ||
| except TypeError: | ||
| self.variants_number = None | ||
|
|
||
| self.trait_reported = trait_reported | ||
| self.trait_efo = trait_efo | ||
| self.trait_mapped = trait_mapped | ||
| self.weight_type = weight_type | ||
| self.citation = citation | ||
| self.HmPOS_build = GenomeBuild.from_string(HmPOS_build) | ||
| self.HmPOS_date = HmPOS_date | ||
| self.format_version = format_version | ||
| self.license = license | ||
|
|
||
| if self.license is None: | ||
| self.license = self._default_license_text | ||
|
|
||
| def __repr__(self): | ||
| values = {x: getattr(self, x) for x in self.fields} | ||
| value_strings = ", ".join([f"{key}='{value}'" for key, value in values.items()]) | ||
| return f"{type(self).__name__}({value_strings})" | ||
|
|
||
| @classmethod | ||
| def from_path(cls, path): | ||
| raw_header: dict = read_header(path) | ||
|
|
||
| if len(raw_header) == 0: | ||
| raise ValueError(f"No header detected in scoring file {path=}") | ||
|
|
||
| header_dict = {k: raw_header.get(k) for k in cls.fields} | ||
|
|
||
| return cls(**header_dict) |
There was a problem hiding this comment.
deleted because migrated to a pydantic model
nebfield
commented
Sep 13, 2024
Comment on lines
+195
to
+212
| @property | ||
| def pgs_id(self): | ||
| return self._pgs_id | ||
|
|
||
| if (build := self._header.HmPOS_build) is not None: | ||
| self.genome_build = build | ||
| self.harmonised = True | ||
| @property | ||
| def is_harmonised(self): | ||
| return self._header.is_harmonised | ||
|
|
||
| @property | ||
| def genome_build(self): | ||
| if self.is_harmonised: | ||
| return self._header.HmPOS_build | ||
| else: | ||
| self.genome_build = self._header.genome_build | ||
| self.harmonised = False | ||
| return self._header.genome_build | ||
|
|
||
| @property | ||
| def header(self): | ||
| return self._header |
There was a problem hiding this comment.
I added these properties because they're a really helpful shortcut in the CLI
nebfield
commented
Sep 13, 2024
Comment on lines
-493
to
-536
| def get_log(self, drop_missing=False, variant_log=None): | ||
| """Create a JSON log from a ScoringFile's header and variant rows.""" | ||
|
|
||
| logger.debug(f"Creating JSON log for {self!r}") | ||
|
|
||
| log = {} | ||
|
|
||
| for attr in self._header.fields: | ||
| if (extracted_attr := getattr(self._header, attr, None)) is not None: | ||
| log[attr] = str(extracted_attr) | ||
| else: | ||
| log[attr] = None | ||
|
|
||
| if log["variants_number"] is None: | ||
| # custom scoring files might not have this information | ||
| log["variants_number"] = variant_log["n_variants"] | ||
|
|
||
| if ( | ||
| variant_log is not None | ||
| and int(log["variants_number"]) != variant_log["n_variants"] | ||
| and not drop_missing | ||
| ): | ||
| logger.warning( | ||
| f"Mismatch between header ({log['variants_number']}) and output row count ({variant_log['n_variants']}) for {self.pgs_id}" | ||
| ) | ||
| logger.warning( | ||
| "This can happen with older scoring files in the PGS Catalog (e.g. PGS000028)" | ||
| ) | ||
|
|
||
| # multiple terms may be separated with a pipe | ||
| if log["trait_mapped"]: | ||
| log["trait_mapped"] = log["trait_mapped"].split("|") | ||
|
|
||
| if log["trait_efo"]: | ||
| log["trait_efo"] = log["trait_efo"].split("|") | ||
|
|
||
| log["columns"] = self._fields | ||
| log["use_harmonised"] = self.harmonised | ||
|
|
||
| if variant_log is not None: | ||
| log["sources"] = [k for k, v in variant_log.items() if k != "n_variants"] | ||
|
|
||
| return {self.pgs_id: log} | ||
|
|
There was a problem hiding this comment.
Replaced by the pydantic models + model_dump()
nebfield
commented
Sep 13, 2024
Comment on lines
384
to
454
| class ScoreVariant(CatalogScoreVariant): | ||
| """This model includes attributes useful for processing and normalising variants | ||
|
|
||
| >>> variant = ScoreVariant(**{"rsID": None, "chr_name": "1", "chr_position": 1, "effect_allele": "A", "effect_weight": 0.5, "row_nr": 0, "accession": "test"}) | ||
| >>> variant # doctest: +ELLIPSIS | ||
| ScoreVariant(rsID=None, chr_name='1', chr_position=1, effect_allele=Allele(allele='A', ... | ||
| >>> variant.is_complex | ||
| False | ||
| >>> variant.is_non_additive | ||
| False | ||
| >>> variant.is_harmonised | ||
| False | ||
| >>> variant.effect_type | ||
| EffectType.ADDITIVE | ||
|
|
||
| >>> variant_missing_positions = ScoreVariant(**{"rsID": None, "chr_name": None, "chr_position": None, "effect_allele": "A", "effect_weight": 0.5, "row_nr": 0, "accession": "test"}) # doctest: +ELLIPSIS | ||
| Traceback (most recent call last): | ||
| ... | ||
| TypeError: Bad position: self.rsID=None, self.chr_name=None, self.chr_position=None | ||
|
|
||
| >>> harmonised_variant = ScoreVariant(**{"rsID": None, "chr_name": "1", "chr_position": 1, "effect_allele": "A", "effect_weight": 0.5, "hm_chr": "1", "hm_pos": 1, "hm_rsID": "rs1921", "hm_source": "ENSEMBL", "row_nr": 0, "accession": "test"}) | ||
| >>> harmonised_variant.is_harmonised | ||
| True | ||
|
|
||
| >>> variant_badly_harmonised = ScoreVariant(**{"rsID": None, "chr_name": "1", "chr_position": 1, "effect_allele": "A", "effect_weight": 0.5, "hm_chr": None, "hm_pos": None, "hm_rsID": "rs1921", "hm_source": "ENSEMBL", "row_nr": 0, "accession": "test"}) | ||
| Traceback (most recent call last): | ||
| ... | ||
| TypeError: Missing harmonised column data: hm_chr | ||
|
|
||
| >>> variant_nonadditive = ScoreVariant(**{"rsID": None, "chr_name": "1", "chr_position": 1, "effect_allele": "A", "effect_weight": 0.5, "dosage_0_weight": 0, "dosage_1_weight": 1, "row_nr": 0, "accession": "test"}) | ||
| >>> variant_nonadditive.is_non_additive | ||
| True | ||
| >>> variant_nonadditive.is_complex | ||
| False | ||
| >>> variant_nonadditive.effect_type | ||
| EffectType.NONADDITIVE | ||
|
|
||
| >>> variant_complex = ScoreVariant(**{"rsID": None, "chr_name": "1", "chr_position": 1, "effect_allele": "A", "effect_weight": 0.5, "is_haplotype": True, "row_nr": 0, "accession": "test"}) | ||
| >>> variant_complex.is_complex | ||
| True | ||
| """ | ||
|
|
||
| model_config = ConfigDict(use_enum_values=True) | ||
|
|
||
| row_nr: int = Field( | ||
| title="Row number", | ||
| description="Row number of variant in scoring file (first variant = 0)", | ||
| ) | ||
| accession: str = Field(title="Accession", description="Accession of score variant") | ||
| is_duplicated: Optional[bool] = Field( | ||
| default=False, | ||
| title="Duplicated variant", | ||
| description="In a list of variants with the same accession, is ID duplicated?", | ||
| ) | ||
|
|
||
| # column names for output are used by __iter__ and when writing out | ||
| output_fields: ClassVar[tuple[str]] = ( | ||
| "chr_name", | ||
| "chr_position", | ||
| "effect_allele", | ||
| "other_allele", | ||
| "effect_weight", | ||
| "effect_type", | ||
| "is_duplicated", | ||
| "accession", | ||
| "row_nr", | ||
| ) | ||
|
|
||
| def __iter__(self): | ||
| for attr in self.output_fields: | ||
| yield getattr(self, attr) |
There was a problem hiding this comment.
thought it would be best to keep all pydantic models in this module 🤔
nebfield
added a commit
that referenced
this pull request
Oct 7, 2024
* add pydantic dependency * implement pydantic model drafts * split models into CatalogScoreVariant and ScoreVariant * fix doctests * refactor to use a DictWriter * fix doctest imports * refactor classes into separate files * add non-additive error checking * skip non-additive scores * add non-additive CLI tests * export EffectTypeError * fix missing CatalogError * tidy up: put reusable pydantic models in models.py and export them * simplify n_finished check * set all effect weight fields to str with coerce_numbers_to_str * add rsid field validator * fix field validator field name o_o * Update score log structure (#49) * include example of validating a scoring file header * update header models * integrate ScoreLog and ScoreLogs * fix doctest imports * add log warning re: missing variants * test log output with incompatible effect type * make ScoreLogs a RootModel to act as a json list * refactor weight types from enum to string * prevent revalidation of ScoreVariants during ScoreLog instantiation * prevent checking position for complex variants * fix log creation with missing variants * update is_non_additive check to support mixed column types * Tidy up: delete unused target variants module (#47) * remove targetvariants class * fix deleted imports * fix imports * Gradually introduce type checking (#50) * set up type checking pre-commit * set up type checks for pgscatalog.core * add mypy action * don't use poetry for mypy * fix is_complex check * delete redundant functions * drop unused NormalisedScoringFile * rename variants -> variant_sources in ScoreLog * add support for ancestry specific allele frequencies * fix writing out * hm_match_chr and hm_match_pos: treat empty strings as None * set up a ProcessPoolExecutor skeleton * fix multiprocessing coverage * convert empty strings to None: is_haplotype and is_diplotype * fix TypeError -> ValueError for pydantic * fix doctest * add support for variant_type field * add complex variant tests * improve model documentation * bump minor version * simplify effect_weight_must_float again * Revert "simplify effect_weight_must_float again" This reverts commit 0d5c6e5. * document and improve effect_weight_must_float * docs: stop being confusing * cache computed fields in model * add more CLI logging messages * add has_complex_alleles computed field * set up annotated types in models and VariantLog model * add package mypy config * ignore typing when reading ScoreVariants as dicts of strings * clarify log messages when writing out
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What's changed
Added pydantic models for:
Integrated these models with the ScoringFile object and combine CLI
There is a breaking change in log structure. Changes include:
compatible_effect_typefield (bool){'PGSXXXX': { nested_dict }) has been made static because of pydantic limitations (replaced with "header" field)Why changed
#46 (comment)
Example log file
PGS000001 is normalised OK and will be in the output, PGS004255 is ignored but warned about.