-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/36796/salisbury new rules #127
base: develop
Are you sure you want to change the base?
Conversation
process_variants(genotype, variant) if positive_record?(genotype) && variant.present? | ||
genotypes.append(genotype) unless test_string.scan(CONFIRM_SEQ_NGS).size.positive? && gene.blank? | ||
def extract_gene_row(genotype, record) | ||
gene = extract_genes(%w[test genotype moleculartestingtype], record) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is moleculartestingtype being used as well here? I can't see it in the rules but please show it to me if I have missed something!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is to capture those recordsin the 'PALB2 data only' and equivalent testing type, Shilpi found they don't always specify PALB2 in the two other fields.
if [2, 10].include? status | ||
assign_variantpathclass_record(genotype_new) | ||
variant = record['genotype'] | ||
process_variants(genotype_new, variant) if positive_record?(genotype_new) && variant.present? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need "if positive_record?" if its in the loop only including statuses 2 and 10
if [2, 10].include? status | ||
assign_variantpathclass_record(genotype_new) | ||
variant = record['genotype'] | ||
process_variants(genotype_new, variant) if positive_record?(genotype_new) && variant.present? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to use 'if positive_record' here? Does that mean we are missing out on capturing variants with test status of 10?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you look at "positive record?" (line 263) it includes those with test status 10 - so maybe the name just needs to be changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I accidentally looked at the method removed instead of the new one. Same comment as Laura's then :-)
if [2, 10].include? status | ||
assign_variantpathclass_record(genotype_new) | ||
variant = record['genotype'] | ||
process_variants(genotype_new, variant) if positive_record?(genotype_new) && variant.present? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you know you are matching the variant with the correct gene here?
What?
Need to update the Salisbury importer code to reflect the new rules to resolve JIRA-2029
Why?
Salisbury has got new gene panels for rules have been added and updated. We also have to accommodate old row level data so whole importer has to be written new way.
How?
We have grouped the Salisbury records for same SRI and treated them row level, panel level or hybrid level as per the rules.
Testing?
Tests have been added and Nichola has signed off QA as the counts match.