Skip to content

Pn/another interpretation refactor#499

Open
psnairne wants to merge 6 commits into
mainfrom
pn/another-interpretation-refactor
Open

Pn/another interpretation refactor#499
psnairne wants to merge 6 commits into
mainfrom
pn/another-interpretation-refactor

Conversation

@psnairne

@psnairne psnairne commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

This PR is broken, because I have not implemented the methods on GIBuilder yet.

What does this PR do?

  1. Makes collection of interpretations more similar to the other collector functions.
  2. Creates a GenomicInterpretationBuilder struct which will handle the building of GenomicInterpretations. Previously this duty was part of PIVOTAL, but the problem with that is that we need to build GenomicInterpretations in a way that is quite specific to our project. So I am going to remove it from PIVOTAL, and transfer it here.
  3. Removes the PathogenicGeneVariantData struct. I think this was confusing, and unnecessary.

Still to do:

  1. Implement the methods on GIBuilder
  2. Fix the tests

Comment on lines +164 to +177
let (allele_one_col, allele_two_col) = match linked_hgvs_cols.len() {
0 => (None, None),
1 => (Some(linked_hgvs_cols[0]), None),
2 => (Some(linked_hgvs_cols[0]), Some(linked_hgvs_cols[1])),
n => {
return Err(CollectorError::ExpectedAtMostNLinkedColumnWithContexts {
table_name: patient_cdf.context().name().to_string(),
bb_id: bb_id.to_string(),
contexts: vec![Context::Hgvs],
n_found: n,
n_expected: 2,
});
}
};

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, this is a simpler, but im unsure if it does the same.

Suggested change
let (allele_one_col, allele_two_col) = match linked_hgvs_cols.len() {
0 => (None, None),
1 => (Some(linked_hgvs_cols[0]), None),
2 => (Some(linked_hgvs_cols[0]), Some(linked_hgvs_cols[1])),
n => {
return Err(CollectorError::ExpectedAtMostNLinkedColumnWithContexts {
table_name: patient_cdf.context().name().to_string(),
bb_id: bb_id.to_string(),
contexts: vec![Context::Hgvs],
n_found: n,
n_expected: 2,
});
}
};
if linked_hgvs_cols.len() > 2 {
return Err(CollectorError::ExpectedAtMostNLinkedColumnWithContexts {
table_name: patient_cdf.context().name().to_string(),
bb_id: bb_id.to_string(),
contexts: vec![Context::Hgvs],
n_found: linked_hgvs_cols.len(),
n_expected: 2,
});
}
let (allele_one_col, allele_two_col) = (
linked_hgvs_cols.first().copied(),
linked_hgvs_cols.last().copied(),
);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your version will return two of the same column when len = 1 which isn't what we want (we want (col, None)).

But I've done something similar now with

        let allele_one_col = linked_hgvs_cols.get(0).copied();
        let allele_two_col = linked_hgvs_cols.get(1).copied();

Comment thread phenoxtract/src/transform/genomic_interpretation_parser.rs
Comment thread phenoxtract/src/transform/collecting/interpretation_collector.rs Outdated
Comment on lines +157 to +158
let linked_hgnc_col =
patient_cdf.get_single_linked_column_as_str(Some(bb_id), &[Context::Hgnc])?;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, I understand this. You are picking a single row and then you always pick row 1 and 2.
This does not seem robust. What if there are three values and the first one is empty?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what you mean.

What happens is this: we allow one linked gene column, and up to two linked HGVS columns. Then we go row by row and collect

gene = Option<&str>, hgvs1 = Option<&str>, hgvs2 = Option<&str>e

It's almost exactly the same as how we do collection for phenotypes and diseases etc. The only difference is there can be between 0 and 2 HGVS columns

Comment thread phenoxtract/src/transform/genomic_interpretation_builder.rs Outdated
Comment thread phenoxtract/Cargo.toml Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants