Allow filtering for several chromosomes #3007
Conversation
|
Ok, it now has the base functionalities:
I think I will call it a day here, but there are some open questions to consider, like how to treat the "All" option?
|
|
This is more or less ready for review, but no rush. I might fix the complex function on a better time of day.. 😬 |
I can review next week! |
Codecov Report
@@ Coverage Diff @@
## main #3007 +/- ##
==========================================
- Coverage 83.87% 83.84% -0.04%
==========================================
Files 287 287
Lines 16474 16484 +10
==========================================
+ Hits 13818 13821 +3
- Misses 2656 2663 +7
Continue to review full report at Codecov.
|
|
No rush, but this should be ready for re-review. I cleaned up the js to decrease complexity, and moved most to a macro, so while it looks like a lot of changes, this is mostly due to the moving of nearly identical scripts from all variantS pages to one macro. |
|
I'll re-review now! |
There was a problem hiding this comment.
The functionality is improving but there are a couple of things to fix I think. The first one is described in my comment (having to manually deselect "All" when you select single chromosomes). This is not something that doesn't work, but an annoying thing that is not going to be appreciated I'm afraid.
One thing that doesn't work is populating the cytobands once you select single chromosomes, have you checked?
Consider using a jquery solution, you could avoid many lines of code and potential errors..
| @@ -192,7 +192,7 @@ text{ | |||
| } | |||
|
|
|||
| ]]></style> | |||
| <script type="text/javascript" xlink:href="javascript/madeline.js"></script> | |||
| <script type="text/javascript" xlink:href="/cases/static/madeline.js"></script> | |||
There was a problem hiding this comment.
I just wanted to keep the console error free by default.. and this should be correct currently, though Im not so sure it is what cg does..
There was a problem hiding this comment.
I'm not sure what that line does 🤔
There was a problem hiding this comment.
It imports some scripts that the svg uses: init() is the one that gets called and reported missing on the console. We serve it as a static from scout, but I think the path might have changed in cg without that being reflected in scout. Regardless, this works, and removes an error when running demo data. The other part would need some investigation on a live case and an issue to cg.
|
I think you will like this one! I swapped from the custom stuff to using the select picker databox select all. |
There was a problem hiding this comment.
Great improvement! Now it works perfectly and it's user-friendly as well!
I have a comment regarding what you pass to the mongo query function, that code-wise would be simpler to just pass a list, even when it contains only one chrom, but if you like it this way it'f fine by me!
Nice new functionality! 🥇
| @@ -248,9 +247,19 @@ def build_query( | |||
| continue | |||
|
|
|||
| if criterion == "chrom" and query.get("chrom"): # filter by coordinates | |||
| query_chrom = query.get("chrom") | |||
| if isinstance(query_chrom, list): | |||
There was a problem hiding this comment.
Wouldn't be easier to just pass a list here, also when there is only one chromosome?
There was a problem hiding this comment.
It would. The only thing is Im reluctant to put in the work to find all places it is called. Maybe I could use like pydantic to recast it. 😁
There was a problem hiding this comment.
Maybe I could use like pydantic to recast it.
You could ask @mikaell that is very familiar with the library 🤣
There was a problem hiding this comment.
If you don't mind I will leave it for another time. I reread the criterion, and it would only shorten that very slightly, as there still is a need for different coordinate handling for SNV and SV on one hand, and for single and multiple chromosomes, with the latter not consistent with start-end coordinates. Looking at it again, I realised we were missing "cancer" in there, so they were always going as SV-overlaps. Should improve performance a little!
There was a problem hiding this comment.
To justify the laziness, the places to look for changes is not trivial. It's not just the forms, its all variants queries e.g.
. Also stored filters would be affected and would probably need resetting or script updating.| @@ -192,7 +192,7 @@ text{ | |||
| } | |||
|
|
|||
| ]]></style> | |||
| <script type="text/javascript" xlink:href="javascript/madeline.js"></script> | |||
| <script type="text/javascript" xlink:href="/cases/static/madeline.js"></script> | |||
There was a problem hiding this comment.
I'm not sure what that line does 🤔
This PR adds a functionality or fixes a bug.
Fix #2971 part 1 (part 2 was solved in another PR).
How to test:
Expected outcome:
The functionality should be working
Take a screenshot and attach or copy/paste the output.
Review: