feat: add mode of inheritance from HPO #493
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #493 +/- ##
====================================
Coverage 74% 74%
====================================
Files 42 43 +1
Lines 7366 7494 +128
====================================
+ Hits 5470 5584 +114
- Misses 1896 1910 +14
|
holtgrewe
force-pushed
the
feat-add-hpo-mode-of-inheritance
branch
from
89fd043 to
adc54f3
Compare
WalkthroughThe changes involve updates to several files within the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Annotator
participant GeneRelatedAnnotation
participant HPO
User->>Annotator: Request for gene annotation
Annotator->>HPO: Load HGNC to Mode of Inheritance mapping
HPO->>Annotator: Return mapping
Annotator->>GeneRelatedAnnotation: Provide gene data and modes of inheritance
GeneRelatedAnnotation->>User: Return annotated gene information
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
src/seqvars/query/annonars.rs (2)
130-131: LGTM: New field correctly added toAnnotatorstruct.The new
hgnc_to_moifield is correctly added and documented. It's consistent with the structure's style, being public like other fields.Consider adding a brief explanation of what MOI stands for in the comment, to improve clarity:
- /// Mapping from HGNC gene ID to modes of inheritance; from `hpo` directory. + /// Mapping from HGNC gene ID to modes of inheritance (MOI); from `hpo` directory.
151-162: LGTM:with_pathmethod correctly updated.The method is properly updated to initialize the new
hgnc_to_moifield. The error handling is comprehensive and consistent with the existing style.For consistency with the error handling for
annonars_dbs, consider wrapping theload_hgnc_to_inheritance_mapcall in a separateResult:let hgnc_to_moi = load_hgnc_to_inheritance_map(&path.as_ref().join("hpo")) .map_err(|e| { anyhow::anyhow!( "problem loading HGNC to mode of inheritance map at {}: {}", path.as_ref().join("hpo").display(), e ) })?;This would make the error handling structure more uniform throughout the method.
src/seqvars/query/hpo.rs (5)
117-117: Use///for field documentation comments.In the
Entrystruct, the fieldhpo_nameis documented using//instead of///. For consistency and to generate proper documentation, please change//to///.Apply this diff to correct the comment:
- // HPO Name. + /// HPO Name.
185-187: Consistent parameter passing forpath.In the function
load_ncbi_to_hgnc, thepathparameter is taken by value (P), whereas other functions likeload_entriestake&P. For consistency and to avoid unnecessary cloning, consider changing the function signature to takepath: &P.Apply this diff to adjust the function signature:
- pub fn load_ncbi_to_hgnc<P: AsRef<std::path::Path>>( - path: P, + pub fn load_ncbi_to_hgnc<P: AsRef<std::path::Path>>( + path: &P,And update the function calls accordingly:
- for entry in load_entries(&path)? { + for entry in load_entries(path)? {
76-81: Include full file paths in error messages.When loading files, it's helpful to include the full file path in error messages to aid in debugging. Consider using
path.as_ref().join(...).display()to show the complete path.Apply this diff to enhance the error messages:
- .map_err(|e| anyhow::anyhow!("error loading phenotype_to_genes.txt: {}", e))?; + .map_err(|e| anyhow::anyhow!( + "error loading {}: {}", + path.as_ref().join("phenotype_to_genes.txt").display(), + e + ))?;Similarly for the next error handling:
- .map_err(|e| anyhow::anyhow!("error loading hgnc_xlink.tsv: {}", e))?; + .map_err(|e| anyhow::anyhow!( + "error loading {}: {}", + path.as_ref().join("hgnc_xlink.tsv").display(), + e + ))?;
203-203: Remove unnecessarypubvisibility in test functions.The test function
test_mode_of_inheritance_from_hpo_idis declared aspub fn, but test functions do not need to be public. Consider changing it tofnto keep the default visibility.Apply this diff to adjust the function definition:
- pub fn test_mode_of_inheritance_from_hpo_id() { + fn test_mode_of_inheritance_from_hpo_id() {
236-236: Remove unnecessarypubvisibility in test functions.The test function
test_mode_of_inheritance_into_pbsis declared aspub fn, but test functions do not need to be public. Consider changing it tofnto keep the default visibility.Apply this diff to adjust the function definition:
- pub fn test_mode_of_inheritance_into_pbs() { + fn test_mode_of_inheritance_into_pbs() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (4)
src/seqvars/query/snapshots/varfish_server_worker__seqvars__query__hpo__test__hgnc_to_xlink_load_entries.snapis excluded by!**/*.snapsrc/seqvars/query/snapshots/varfish_server_worker__seqvars__query__hpo__test__load_hgnc_to_inheritance_map.snapis excluded by!**/*.snapsrc/seqvars/query/snapshots/varfish_server_worker__seqvars__query__hpo__test__phenotype_to_genes_load_entries.snapis excluded by!**/*.snaptests/strucvars/query/db/hpo/hgnc_xlink.tsvis excluded by!**/*.tsv
📒 Files selected for processing (6)
- protos/varfish/v1/seqvars/output.proto (1 hunks)
- src/seqvars/query/annonars.rs (3 hunks)
- src/seqvars/query/hpo.rs (1 hunks)
- src/seqvars/query/mod.rs (3 hunks)
- src/strucvars/aggregate/cli.rs (1 hunks)
- tests/strucvars/query/db/hpo/phenotype_to_genes.txt (1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/strucvars/aggregate/cli.rs
- tests/strucvars/query/db/hpo/phenotype_to_genes.txt
🧰 Additional context used
🔇 Additional comments (10)
src/seqvars/query/annonars.rs (2)
9-12: LGTM: New imports are correctly added.The new imports from the
hpomodule are necessary for the added functionality and follow the existing import style.
Line range hint
1-314: Summary: Changes enhanceAnnotatorwith mode of inheritance functionality.The modifications to
src/seqvars/query/annonars.rssuccessfully integrate mode of inheritance data into theAnnotatorstruct. The changes are well-implemented, maintaining consistency with the existing code style and error handling patterns. Minor suggestions for documentation and code structure improvements have been provided, but overall, the implementation is sound and ready for integration.protos/varfish/v1/seqvars/output.proto (3)
170-185: LGTM: Well-structured enum for modes of inheritanceThe new
ModeOfInheritanceenum is well-defined and covers the main modes of inheritance relevant to genetic studies. The inclusion of HPO IDs in comments adds valuable cross-referencing information. The enum follows Proto3 best practices by including an UNSPECIFIED option with value 0.
193-194: LGTM: Appropriate addition of modes_of_inheritance fieldThe addition of the
modes_of_inheritancefield to theGeneRelatedPhenotypesmessage is well-implemented. Using a repeated field of typeModeOfInheritanceallows for the association of multiple inheritance modes with a gene-related phenotype, enhancing the data model with more detailed genetic information.
170-194: Summary: Excellent addition of mode of inheritance informationThe changes in this file successfully implement the addition of mode of inheritance information from HPO. The new
ModeOfInheritanceenum and themodes_of_inheritancefield in theGeneRelatedPhenotypesmessage work together to enhance the data model with valuable genetic inheritance information. These modifications align well with the PR objectives and follow Proto3 best practices.The implementation allows for:
- Clear representation of various modes of inheritance with corresponding HPO IDs.
- Association of multiple inheritance modes with gene-related phenotypes.
These changes will improve the ability to analyze and interpret genetic data within the VarFish system.
src/seqvars/query/mod.rs (5)
4-4: Importing the 'hpo' moduleThe
hpomodule is correctly imported to handle Human Phenotype Ontology functionalities.
716-716: Retrieving modes of inheritance for genesThe code appropriately retrieves the modes of inheritance (
mois) for the gene using the annotator.
724-724: Passingmoisto thephenotypesfunctionThe
moisparameter is correctly passed to thephenotypesfunction, enabling modes of inheritance to be included in the phenotype annotations.
761-761: Updatingphenotypesfunction signatureThe
phenotypesfunction signature is updated to acceptmois, allowing it to process modes of inheritance.
768-773: Including modes of inheritance in phenotype annotationsThe code correctly processes
moisand includes the modes of inheritance in theGeneRelatedPhenotypesoutput.
src/seqvars/query/hpo.rs
Outdated
| let modes_of_inheritance = result | ||
| .entry(hgnc_id.clone()) | ||
| .or_insert_with(indexmap::IndexSet::new); | ||
| modes_of_inheritance.insert_sorted(mode_of_inheritance); |
There was a problem hiding this comment.
IndexSet does not have an insert_sorted method.
The method insert_sorted is being called on an IndexSet, but IndexSet does not provide an insert_sorted method. This will result in a compilation error. Consider using the insert method instead to add elements to the set.
Apply this diff to fix the method call:
- modes_of_inheritance.insert_sorted(mode_of_inheritance);
+ modes_of_inheritance.insert(mode_of_inheritance);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| modes_of_inheritance.insert_sorted(mode_of_inheritance); | |
| modes_of_inheritance.insert(mode_of_inheritance); |
There was a problem hiding this comment.
You are wrong, IndexSet, has such a function.
There was a problem hiding this comment.
@holtgrewe, thank you for the correction. You're right; IndexSet does have an insert_sorted method. I apologize for the confusion.
🐰🧐
✏️ Learnings added
Learnt from: holtgrewe
PR: varfish-org/varfish-server-worker#493
File: src/seqvars/query/hpo.rs:0-0
Timestamp: 2024-10-09T12:42:38.460Z
Learning: In Rust, the `IndexSet` type from the `indexmap` crate provides an `insert_sorted` method.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Summary by CodeRabbit
Release Notes
New Features
Annotatorwith a mapping from HGNC gene IDs to modes of inheritance.Bug Fixes
Chores