Fix filters parse and display #5061

dnil · 2024-11-22T09:49:19Z

This PR adds a functionality or fixes a bug.

Fix Missing filter status? #5060

Bit of a difference already without touching parsing:

Then with parsing fixed:

I also added a filters badge to the variant page for good measure:

Testing on cg-vm1 server (Clinical Genomics Stockholm)

Prepare for testing

Make sure the PR is pushed and available on Docker Hub
Fist book your testing time using the Pax software available at https://pax.scilifelab.se/. The resource you are going to call dibs on is scout-stage and the server is cg-vm1.
ssh <USER.NAME>@cg-vm1.scilifelab.se
sudo -iu hiseq.clinical
ssh localhost
(optional) Find out which scout branch is currently deployed on cg-vm1: podman ps
Stop the service with current deployed branch: systemctl --user stop scout.target
Start the scout service with the branch to test: systemctl --user start scout@<this_branch>
Make sure the branch is deployed: systemctl --user status scout.target
After testing is done, repeat procedure at https://pax.scilifelab.se/, which will release the allocated resource (scout-stage) to be used for testing by other users.

Testing on hasta server (Clinical Genomics Stockholm)

Prepare for testing

ssh <USER.NAME>@hasta.scilifelab.se
Book your testing time using the Pax software. us; paxa -u <user> -s hasta -r scout-stage. You can also use the WSGI Pax app available at https://pax.scilifelab.se/.
(optional) Find out which scout branch is currently deployed on cg-vm1: conda activate S_scout; pip freeze | grep scout-browser
Deploy the branch to test: bash /home/proj/production/servers/resources/hasta.scilifelab.se/update-tool-stage.sh -e S_scout -t scout -b <this_branch>
Make sure the branch is deployed: us; scout --version
After testing is done, repeat the paxa procedure, which will release the allocated resource (scout-stage) to be used for testing by other users.

How to test:

check a cancer case and an RD case, note filter tags missing and tags saying Pass when they should say Filtered.
apply patch
note improvements with old cases, e.g. some new danger badges on variantS, and the filters badge on the variant page. And in particular no new issues with them.
for full effect, load/update the cases with the patch and note good tags both for callers and filters

Expected outcome:
The functionality should be working
Take a screenshot and attach or copy/paste the output.

Review:

code approved by
tests executed by

dnil · 2024-11-22T09:57:15Z

Ok, with that out of the world, I would say we are not formally bugged at least not based on the demo case situation.

Since the set tag is used without a "filteredIn", it is hard to fault the parsing here. This looks like more like a merging/pipeline issue.

But we could perhaps do slightly better here: since there is only one caller given, we could assign the FILTER status to the call I think!

codecov · 2024-11-22T09:58:35Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.81%. Comparing base (53f537b) to head (95ee422).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5061      +/-   ##
==========================================
+ Coverage   84.78%   84.81%   +0.02%     
==========================================
  Files         323      323              
  Lines       19442    19475      +33     
==========================================
+ Hits        16484    16517      +33     
  Misses       2958     2958

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

… on variant page

dnil · 2024-11-22T12:48:27Z

scout/parse/variant/callers.py


    if raw_info or svdb_origin or other_info:
        return callers

    if category == "snv":
-        # cyvcf2 FILTER is None if VCF file column FILTER is "PASS"
        filter_status = "Pass"
        if variant.FILTER is not None:
            filter_status = "Filtered - {}".format(filter_status.replace(";", " - "))


Note that filter_status could never take any other value than "Pass" here really, so it was either "Pass" or "Filtered - Pass" I guess?!

dnil · 2024-11-22T13:36:43Z

scout/parse/variant/callers.py

@@ -13,6 +13,14 @@ def parse_callers(variant, category="snv"):
    2. If a svdb_origin tag (pipe separated) is found, callers listed will be marked Pass
    3. If a set tag (dash separated, GATK CombineVariants) is found, callers will be marked Pass or Filtered accordingly

+    If the FILTER status is not PASS (e.g. None - cyvcf2 FILTER is None if VCF file column FILTER is "PASS"),


I dunno, this function is getting long. If I didn't have the thought about a new parsing module nudging I might split/refactor it. Say if you feel it is way long already.

Agreed, this function might be split into 3 more functions, one for each of the 3 situations:

1. If a FOUND_IN tag (comma separated) is found, callers listed will be marked Pass 2. If a svdb_origin tag (pipe separated) is found, callers listed will be marked Pass 3. If a set tag (dash separated, GATK CombineVariants) is found, callers will be marked Pass or Filtered accordingly

I guess porting it will be easier if it is somewhat clear; refactored into four separate cases (one for the gatk snv fallback as well).

dnil · 2024-11-22T13:38:25Z

scout/parse/variant/callers.py

@@ -28,36 +36,51 @@ def parse_callers(variant, category="snv"):
    svdb_origin = variant.INFO.get("svdb_origin")
    raw_info = variant.INFO.get("set")

+    filter_status_default = "Pass"
+    if variant.FILTER is not None:
+        filter_status_default = "Filtered - {}".format(variant.FILTER.replace(";", " - "))


This is one of the crucial changes. As noted below at its origin, the previous version would never add the actual FILTER tag on parsing. Python lazy initiation error. That would just have been a compile error in another language.

dnil · 2024-11-22T13:39:37Z

scout/parse/variant/callers.py

    if other_info:
-        for info in other_info.split(","):
+        infos = other_info.split(",")
+        if len(infos) > 1:


As per the doctoring: add filter status if we are looking at a single caller, else just set as before.

dnil · 2024-11-22T13:43:07Z

scout/server/blueprints/variants/templates/variants/utils.html

@@ -56,7 +56,7 @@
      </span>
    {% endfor %}
  {% for name, caller in variant.callers %} <!-- Collect info for specific callers -->
-      <span class="badge {% if caller == 'Pass' %}bg-success{% elif caller == 'Filtered' %}bg-secondary{% else %}bg-black{% endif %}" data-bs-toggle="tooltip" data-bs-html="true" title="{{caller}}">
+      <span class="badge {% if caller == 'Pass' %}bg-success{% elif 'Filtered' in caller %}bg-secondary{% else %}bg-black{% endif %}" data-bs-toggle="tooltip" data-bs-html="true" title="{{caller}}">


Show if substring matches, as in "Filtered - MinSomaticScore".

northwestwitch · 2024-11-25T07:03:53Z

Just to understand, when there is filter passed but not a specific caller, then originally the filter was not "Passed" but "filtered"?

northwestwitch

This seems to capture more filtered events, as oppsite to what happens on main branch, which is nice!
Would be nice to:

Split the parsing function into different smaller functions, as you also suggested.
Fix the filter label displayed on variantS page, because it's too long for that cell.

I'm still trying to understand why we still have the green PASS label with or without callers for some cancer variants. I guess I have to look inside the VCF

northwestwitch · 2024-11-25T07:21:44Z

scout/parse/variant/callers.py

@@ -13,6 +13,14 @@ def parse_callers(variant, category="snv"):
    2. If a svdb_origin tag (pipe separated) is found, callers listed will be marked Pass
    3. If a set tag (dash separated, GATK CombineVariants) is found, callers will be marked Pass or Filtered accordingly

+    If the FILTER status is not PASS (e.g. None - cyvcf2 FILTER is None if VCF file column FILTER is "PASS"),


Agreed, this function might be split into 3 more functions, one for each of the 3 situations:

1. If a FOUND_IN tag (comma separated) is found, callers listed will be marked Pass 2. If a svdb_origin tag (pipe separated) is found, callers listed will be marked Pass 3. If a set tag (dash separated, GATK CombineVariants) is found, callers will be marked Pass or Filtered accordingly

scout/parse/variant/callers.py

northwestwitch · 2024-11-25T07:42:01Z

scout/parse/variant/callers.py

@@ -45,19 +63,21 @@ def parse_callers(variant, category="snv"):
                    callers[caller] = "Filtered"
            elif call == "Intersection":
                for caller in callers:
-                    callers[caller] = "Pass"
+                    callers[caller] = filter_status_default


Can you have variant.FILTER not None and raw_info? Sorry, I'm trying to wrap my head around these conditions..

scout/server/blueprints/variant/utils.py

dnil · 2024-11-25T15:36:17Z

I'm still trying to understand why we still have the green PASS label with or without callers for some cancer variants. I guess I have to look inside the VCF

Yes, there are some lines there that appear odd to my understanding, much like if it was an amalgamation of different pipeline runs or call sets. I guess the demo is a) old and b) perhaps not entirely representative of one pipeline version, but has some edited lines etc.

dnil · 2024-11-25T16:14:41Z

Just to understand, when there is filter passed but not a specific caller, then originally the filter was not "Passed" but "filtered"?

Hm, no, I thought it would have been Pass and None on all callers for the category?

I think you are clear on this, but to be sure, and to remember later, the issues were in particular

If a filter status was new/missing from a scout constant VARIANT_FILTERS of known (mostly Lund specific) filter strings, the filter status badge was not shown.
For variants with variant.FILTER status, callers are sometimes marked, e.g. with INFO.FOUND_IN. That was likely not what we expected from the pipeline side, but I guess it makes some sense as well, to tell which tool made the call that got filtered. These would previously be marked with caller status Pass even if a variant.FILTER status was set.

northwestwitch

Nice with the refactoring and new tests! 💯

CHANGELOG.md

scout/parse/variant/callers.py

tests/parse/test_parse_callers.py

sonarcloud · 2024-11-26T08:25:47Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

dnil and others added 3 commits November 22, 2024 10:18

style cancer vars subcategory upper en passant

e0336ff

Fix #5060 - first, display unknown tags as danger badges

4f53230

Fix code style issues with Black

959b274

dnil marked this pull request as draft November 22, 2024 09:49

dnil and others added 4 commits November 22, 2024 11:36

improve parsing - set filter status exact if single caller, show also…

4dfd827

… on variant page

refactor to function and show on variant page as well

bd0c34a

Fix code style issues with Black

aba59d0

less naively, but that was actually bugged from the get-go

287b42b

dnil commented Nov 22, 2024

View reviewed changes

dnil added 2 commits November 22, 2024 14:04

name colission

30ea744

Filtered in all would not really ever know where add the FILTER tag

333d028

dnil marked this pull request as ready for review November 22, 2024 13:35

dnil commented Nov 22, 2024

View reviewed changes

could just as well revert that filteredinall

1cb3201

dnil commented Nov 22, 2024

View reviewed changes

whitespace

4b40928

dnil added the Bugfix label Nov 22, 2024

Merge branch 'main' into fix_filters_parse_and_display

e3bff2e

northwestwitch requested changes Nov 25, 2024

View reviewed changes

dnil and others added 6 commits November 25, 2024 10:08

Merge branch 'main' into fix_filters_parse_and_display

a43b2ed

Merge branch 'main' into fix_filters_parse_and_display

d3d7e22

Merge branch 'main' into fix_filters_parse_and_display

7d4f590

refactor parse_callers

4753e35

ahem, refactor parse_callers

ad2c28a

Fix code style issues with Black

c30351c

dnil added 3 commits November 25, 2024 16:50

crowded callers cell with many or long filters

68e200b

add a couple of tests

94c90c2

format string constant

c66027a

do return

92e58a0

dnil requested a review from northwestwitch November 25, 2024 16:44

northwestwitch approved these changes Nov 26, 2024

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

scout/parse/variant/callers.py Show resolved Hide resolved

scout/parse/variant/callers.py Outdated Show resolved Hide resolved

tests/parse/test_parse_callers.py Show resolved Hide resolved

tests/parse/test_parse_callers.py Show resolved Hide resolved

dnil added 2 commits November 26, 2024 09:15

Use string constants

3f0a802

slightly prettier descriptions in tooltip

95ee422

dnil merged commit 6c115e8 into main Nov 26, 2024
25 checks passed

dnil deleted the fix_filters_parse_and_display branch November 26, 2024 08:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix filters parse and display #5061

Fix filters parse and display #5061

dnil commented Nov 22, 2024 •

edited

Loading

dnil commented Nov 22, 2024 •

edited

Loading

codecov bot commented Nov 22, 2024 •

edited

Loading

dnil Nov 22, 2024

dnil Nov 22, 2024

northwestwitch Nov 25, 2024

dnil Nov 25, 2024

dnil Nov 22, 2024

dnil Nov 22, 2024

dnil Nov 22, 2024

northwestwitch commented Nov 25, 2024

northwestwitch left a comment •

edited

Loading

northwestwitch Nov 25, 2024

northwestwitch Nov 25, 2024

dnil commented Nov 25, 2024

dnil commented Nov 25, 2024 •

edited

Loading

northwestwitch left a comment

sonarcloud bot commented Nov 26, 2024

Fix filters parse and display #5061

Fix filters parse and display #5061

Conversation

dnil commented Nov 22, 2024 • edited Loading

dnil commented Nov 22, 2024 • edited Loading

codecov bot commented Nov 22, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

northwestwitch commented Nov 25, 2024

northwestwitch left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnil commented Nov 25, 2024

dnil commented Nov 25, 2024 • edited Loading

northwestwitch left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Nov 26, 2024

Quality Gate passed

dnil commented Nov 22, 2024 •

edited

Loading

dnil commented Nov 22, 2024 •

edited

Loading

codecov bot commented Nov 22, 2024 •

edited

Loading

northwestwitch left a comment •

edited

Loading

dnil commented Nov 25, 2024 •

edited

Loading