Multiple variant tags filtering #3502

ShifaSZ · 2023-07-20T15:41:56Z

Now, it updates the saved-variant filtering with multiple tags. No behavior changes to the project saved-variants.

ShifaSZ · 2023-07-27T15:34:40Z

seqr/views/apis/summary_data_api.py

+        saved_variant_models = SavedVariant.objects.filter(
+            varianttag__variant_tag_type__name__in=tags,
+            varianttag__variant_tag_type__project__isnull=True,


It returns more saved variants than we need. But the front end will filter out the proper variants to display.

we should be doing all the filtering on the backend for the summary data page. Why not keep the 2 distinct queries we had before so that we actually only retunr the correct variants:

tag_types = VariantTagType.objects.filter(name__in=tags, project__isnull=True) saved_variant_models = SavedVariant.objects.filter(varianttag__variant_tag_type__in=tag_types)

tag_types = VariantTagType.objects.filter(name__in=tags, project__isnull=True)
saved_variant_models = SavedVariant.objects.filter(varianttag__variant_tag_type__in=tag_types)

I did some tests and found the result is the same. I can't find a Django filter that means our requirements. What I could do is add a Python filter afterward.

can't you just chain the filters?

saved_variant_models = SavedVariant.objects for tag_type in tag_types: saved_variant_models = saved_variant_models.filter(varianttag__variant_tag_type=tag_type)

It works. Can it be a concern about having many DB queries?

django converts that to 1 db query. Chaining filter statements does not execute a query

ShifaSZ · 2023-07-27T15:39:51Z

ui/pages/SummaryData/reducers.js

+      } else if (gene) {
+        dispatch({
+          type: RECEIVE_SAVED_VARIANT_TAGS,
+          updates: SUMMARY_PAGE_SAVED_VARIANT_TAGS.reduce((acc, t) => ({ ...acc, [t]: false }), {}),


Clear loaded tags flags. It is better to detect gene changes. But I wonder if it's worth doing.

why would we clear previously loaded data because we loaded more data?

All the data loaded for the tags must be reloaded if the gene changes because the data differs for the genes with the same tags. I don't detect gene changes (I probably should). So clear tags loaded flag if the gene is not null to ensure correct results.

but if we previously loaded like all the variants for a different tag, why would clear that? We should only clear for the tag(s) that were loaded

Should we change the keys from tags to tag and gene pairs? It is cleaner and has better performance.

seqr/views/apis/summary_data_api.py

hanars · 2023-07-27T16:00:39Z

seqr/views/apis/summary_data_api.py

+        saved_variant_models = SavedVariant.objects.filter(
+            varianttag__variant_tag_type__name__in=tags,
+            varianttag__variant_tag_type__project__isnull=True,


we should be doing all the filtering on the backend for the summary data page. Why not keep the 2 distinct queries we had before so that we actually only retunr the correct variants:

tag_types = VariantTagType.objects.filter(name__in=tags, project__isnull=True) saved_variant_models = SavedVariant.objects.filter(varianttag__variant_tag_type__in=tag_types)

hanars · 2023-07-27T16:02:57Z

ui/shared/utils/constants.js

@@ -1041,6 +1041,29 @@ export const REVIEW_TAG_NAME = 'Review'
 export const KNOWN_GENE_FOR_PHENOTYPE_TAG_NAME = 'Known gene for phenotype'
 export const DISCOVERY_CATEGORY_NAME = 'CMG Discovery Tags'
 export const MME_TAG_NAME = 'MME Submission'
+export const SUMMARY_PAGE_SAVED_VARIANT_TAGS = [


it is confusing to have a page-specific constant in the global constants list, exspecially sicnce its used in other forms elsewhere. It is better to have a single VARIANT_TAGS constant and then if there need to be different tag options on different pages you can filter out the ones you don;t want for that specific page

The dropdown tag options for the summary page and project page are different.
On the summary page, they are as the following figure:

On the project pages, they are:

there should not be a page specific constant in the shared constants file. If things are different on different pages, there should be page specific constants in the page specific constants files. If this constant is used in multiple places, then it should not be named SUMMARY_PAGE_SAVED_VARIANT_TAGS it should just be SAVED_VARIANT_TAGS. If it is just used in one place for one page, then it should be in a different file

Move the constant into the SummaryData folder.

hanars · 2023-07-27T16:05:38Z

ui/pages/SummaryData/reducers.js

    (responseJson) => {
      if (tag && !gene) {
+        if (tags[0] === SHOW_ALL) {


this case is impossible - you can't return all variants with no gene filter applied

hanars · 2023-07-27T16:06:08Z

ui/pages/SummaryData/reducers.js

+      } else if (gene) {
+        dispatch({
+          type: RECEIVE_SAVED_VARIANT_TAGS,
+          updates: SUMMARY_PAGE_SAVED_VARIANT_TAGS.reduce((acc, t) => ({ ...acc, [t]: false }), {}),


why would we clear previously loaded data because we loaded more data?

ShifaSZ · 2023-08-03T21:33:37Z

ui/pages/SummaryData/constants.js

-/* eslint-disable import/prefer-default-export */
-


There is a warning that the eslint-disable doesn't change anything because there are no import/prefer-default-export violations.

ShifaSZ · 2023-08-03T21:58:53Z

ui/pages/Project/components/SavedVariants.jsx

-    const isCategory = categoryOptions.includes(newTag)
-    updateTableField('categoryFilter')(isCategory ? newTag : null)
+    const lastNewTag = newTag.length > 0 ? newTag[newTag.length - 1] : null
+    const isCategory = categoryOptions.includes(lastNewTag)
+    const [firstTag, ...otherTag] = newTag
+    const updatedTag = firstTag === ALL_FILTER || categoryOptions.includes(firstTag) ? otherTag : newTag
+    updateTableField('categoryFilter')(isCategory ? lastNewTag : null)


The behaviors include: if the latest selected option is a category, remove all selected options; otherwise if the previous (or the first) selected option is a category, remove the category option.
The issue is that when a category is selected, the category disappears from the option list.
No category is selected:

After a category (Collaboration) is selected:

if a category is selected, all the tags in the category should be shown as selected and should be added to the list of selected tag breadcrumbs, the category itself should remain in the dropdown and clicking it again will have no effect

Also, selected tags need to be the color of the tag, similar to how they behave in the UI for adding tags to a variant

the category itself should remain in the dropdown and clicking it again will have no effect

How about the category also disappear from the dropdown list? It is much easier to implement and it doesn't make sense to click on a selected category.

ShifaSZ · 2023-08-03T22:03:24Z

ui/pages/SummaryData/reducers.js

+  const stateKey = tag && `${tag.split(TAG_URL_DELIMITER).sort().join(TAG_URL_DELIMITER)}${gene}`
  if (tag) {
-    if (getState().savedVariantTags[tag]) {
+    if (getState().savedVariantTags[stateKey]) {


Add the gene to the key for the state so that no states have to be removed when gene changed.

this seems to be adding a lof of complexity that will be hard to maintain and have almost no performance improvement - we don't anticipate users toggling back and forth to the same gene, so refetching a gene is not an issue. Please explain why this was not needed before but is now needed because we are adding support for multiple tags

It was a bug before when the gene changed. The UI still displayed the variant data for the previous gene.

then aither a) make a separate PR to fix the bug and branch from that. or b) make a ticket to fix that bug and fix it later and not as part of this PR. This PR is really hard to understand because there is a lot in here that is unrelated to the functionality change requested in the ticket, and I can't review it effectively

I can leave the reducer unchanged for this PR and create a bug ticket.

hanars · 2023-08-04T14:20:23Z

ui/pages/SummaryData/components/SavedVariants.jsx

@@ -29,29 +29,7 @@ const FILTER_FIELDS = [
  VARIANT_PER_PAGE_FIELD,
 ]

-const TAG_OPTIONS = [


what was the advantage of moving this? The constant is only ever used here so it is better to keep it in the file. In general, PRs should really focus on changing the behavior needed for the actual functional change, adding unrelated changes makes reviewing the actual functional changes harder

Oh, you are right. I used the tag options for the reducer to update the tag-loaded state before my latest update. We don't need it now.

hanars

This PR has gotten unwieldy, in large part because the scope has been really blown up. The initial ticket requested only that behavior for the summary data page change. In an attempt to simplify the change we decided to also change the functionality on the project page, but this has gotten way too complicated in how categories will be handled etc. I am restricting the scope of this back to only the summary page, ad the project page will continue to be a single select

hanars · 2023-08-07T17:30:05Z

seqr/views/apis/summary_data_api.py

-        tag_type = VariantTagType.objects.get(name=tag, project__isnull=True)
-        saved_variant_models = SavedVariant.objects.filter(varianttag__variant_tag_type=tag_type)
+        tags = tag.split(';')
+        tag_type = VariantTagType.objects.filter(name__in=tags, project__isnull=True)


should be tag_types plural, as it is a collection of models not a single model

seqr/views/apis/summary_data_api_tests.py

hanars · 2023-08-07T17:33:16Z

seqr/views/apis/summary_data_api_tests.py

@@ -231,14 +236,18 @@ class LocalSummaryDataAPITest(AuthenticationTestCase, SummaryDataAPITest):
    MANAGER_VARIANT_GUID = 'SV0000006_1248367227_r0004_non'


-def assert_has_expected_calls(self, users, skip_group_call_idxs=None):
+def assert_has_expected_calls(self, users, skip_group_call_idxs=None, has_ws_access_level_call=False):


this function is only used twice so theres no reused behavior for mock_get_ws_access_level at all anymore - it wold be better to take that check out of the helper and explicitly add the correct check for it after calling this helper

hanars · 2023-08-07T17:44:17Z

ui/shared/components/panel/variants/SavedVariants.jsx

+    const tags = tag ? tag.split(TAG_URL_DELIMITER) : tag
+    const tagCategoryFilters = tags || (variantGuid ? [] : [(tableState.categoryFilter || ALL_FILTER)])
+    const firstAppliedTagCategoryFilter = tagCategoryFilters.length > 0 ? tagCategoryFilters[0] : null
+    const appliedTagCategoryFilter = multiple ? tagCategoryFilters : firstAppliedTagCategoryFilter


this functionality should not be shared, its really page specific and hard to understand whats happening here. Having a dropdownValue prop explicitly passed through here would make the page specific logic much clearer

hanars · 2023-08-07T17:47:01Z

ui/shared/components/panel/variants/SavedVariants.jsx


-    let shownFilters = (discoveryFilters && appliedTagCategoryFilter === DISCOVERY_CATEGORY_NAME) ?
+    let shownFilters = (discoveryFilters && firstAppliedTagCategoryFilter === DISCOVERY_CATEGORY_NAME) ?


similar to above, discoveryFilters is page specific so its confusing to have this logic here, it would be better to move this logic into the page-specific component and conditionally pass the correct filters prop

hanars · 2023-08-07T17:53:03Z

ui/shared/components/panel/variants/selectors.js

-      pairedVariants = matchingVariants(
-        pairedVariants, ({ tagGuids }) => tagGuids.some(tagGuid => tagsByGuid[tagGuid].name === tag),
-      )
+      const tags = tag.split(TAG_URL_DELIMITER)


Doesn't doing the split here means all the above logic for MME/notes would not work?

MME Submission/Has Notes won't work for multiple-tag filtering. They are not required since they are not on the summary data options list. But we should have a more systemic solution for handling tag/tags here.

Since we filter the tags in the backend and don't support the project saved-variant multiple-tag filtering, these changes are unnecessary.

ShifaSZ · 2023-08-09T17:50:13Z

ui/shared/components/panel/variants/SavedVariants.jsx

-
-    const appliedTagCategoryFilter = tag || (variantGuid ? null : (tableState.categoryFilter || ALL_FILTER))
+    const { familyGuid, variantGuid } = match.params
+    const optionValue = getSelectedTag(variantGuid ? null : (tableState.categoryFilter || ALL_FILTER))


Use a function prop to get the selected option value because the tableState is only available here.

getSelectedTag should then take tableState as an argument and have page specific handling for that, as this is still page specific logic. Moreover, variantGuid is only a valid prop on the project page, not the summary page, so that logic also belongs in the project page

hanars · 2023-08-10T15:41:03Z

ui/pages/SummaryData/components/SavedVariants.jsx

@@ -67,11 +68,17 @@ TAG_OPTIONS.push({

 const PAGE_URL = '/summary_data/saved_variants'

-const getUpdateTagUrl =
-  (selectedTag, match) => `${PAGE_URL}/${selectedTag}${match.params.gene ? `/${match.params.gene}` : ''}`
+const getUpdateTagUrl = (selectedTag, match) => {


A multiple dropdown should have a list as its value, and it should therefore handle adding or removing elements to that list smoothly, and you should not be writing this sort of custom logic

All the custom logic is about the SHOW_ALL option. We need to remove the existing SHOW_ALL when adding a new option and remove all current options if the last option is SHOW_ALL.

show all should not act like a multi-option - there should be no way to select "show all" and also select other tags. Is there a reason users should even have the show all option anymore on the summary page? Presumably, if they send a request with no tags we will return results for all tags, so just have "All" be something thats shown by default when the tag list is empty should be suufficient

hanars · 2023-08-10T15:43:15Z

ui/shared/components/panel/variants/SavedVariants.jsx

-
-    const appliedTagCategoryFilter = tag || (variantGuid ? null : (tableState.categoryFilter || ALL_FILTER))
+    const { familyGuid, variantGuid } = match.params
+    const optionValue = getSelectedTag(variantGuid ? null : (tableState.categoryFilter || ALL_FILTER))


getSelectedTag should then take tableState as an argument and have page specific handling for that, as this is still page specific logic. Moreover, variantGuid is only a valid prop on the project page, not the summary page, so that logic also belongs in the project page

hanars · 2023-08-10T15:45:05Z

ui/shared/components/panel/variants/selectors.js

+    if (tags.length > 1) {
+      pairedVariants = matchingVariants(
+        pairedVariants, ({ tagGuids }) => {
+          const tagNames = tagGuids.map(tagGuid => tagsByGuid[tagGuid].name)
+          return tags.every(tagName => tagNames.includes(tagName))


Why? The combined DB filter you had implemented in the last round of review worked?

ShifaSZ · 2023-08-10T21:36:42Z

ui/pages/SummaryData/reducers.js

      dispatch({ type: RECEIVE_DATA, updatesById: responseJson })
+      dispatch({ type: RECEIVE_SAVED_VARIANT_TAGS, updates: { [tag]: responseJson.savedVariantsByGuid } })


It works but is risky because the first dispatch stops the loading state, and the second dispatch update the data state later. But it won't work if we do it in another order because dispatching RECEIVE_SAVED_VARIANT_TAGS triggers the selector running, which needs the data from the other dispatch.
Are there ways to do these two dispatches together?

Are there ways to do these two dispatches together?

yes, instead of making the new state field have its own action type the way you do here, have it use RECEIVE_DATA as well. However you should not be storing the exact same data twice. Just save the guids associated with the compound tag and then get the full variants in the selector
Note: edited because my first suggestion was unneccessarily cmplex

dispatch({ type: RECEIVE_DATA, updatesById: { ...responseJson, multiTagVariants: { [tag]: Object.keys(responseJson.savedVariantsByGuid) } }) ... // In reducer definition savedVariantsByTag: createObjectsByIdReducer(RECEIVE_DATA, 'multiTagVariants'),

hanars · 2023-08-11T14:40:11Z

ui/shared/components/panel/variants/selectors.test.js

@@ -67,7 +79,7 @@ test('getPairedFilteredSavedVariants', () => {

 test('getVisibleSortedSavedVariants', () => {
  const savedVariants = getVisibleSortedSavedVariants(
-    STATE_WITH_2_FAMILIES, { match: { params:  {} } }
+    STATE_WITH_2_FAMILIES, { project: { projectGuid: 'R0237_1000_genomes_demo' }, match: { params:  {} } },


we should also be testing without the project and with the new state field to check that that works correctly too

hanars · 2023-08-11T19:38:16Z

ui/pages/SummaryData/reducers.js

-      dispatch({ type: RECEIVE_DATA, updatesById: responseJson })
+      dispatch({
+        type: RECEIVE_DATA,
+        updatesById: { ...responseJson, multiTagVariants: { [tag]: Object.keys(responseJson.savedVariantsByGuid) } },


if (tag && !gene) still seems like an important conditional, why was it removed?

I proposed naming this multiTagVariants because I thought this would be special-cased only for multi variant tags, but this implementation is nicer. However, I would rename the key savedVariantsByTag

if (tag && !gene) still seems like an important conditional, why was it removed?

Not do caching when there is a gene? It makes sense. I could add the condition to the value of the savedVariantsByTag generation. But it still has corner cases for switching from no gene to having a gene.

How is that a corner case? The filtering in the selector will work fine for that

When switching from no gene to having a gene, we should ignore the cache and reload data for the summary page. We can add a !gene condition to the data loaded check in the reducer. But the data loaded check will fail when it switches back to no gene.
Another case is that if the cached data is loaded for a particular gene, the selector may mistakenly use it for the project page for any gene.

I want to add the gene to the key to solve the issues. So the savedVariantsByTag will become savedVariantsByTagGene.

the amount we are caching is overkill. This is too complex and it will save basically no time for the user. Do not add this additional level of caching. Just do this like search results, where theres a single state key with a list (for search its searchedVariants, here it will be something like summaryDataVariants) that just gets overwritten every time new summary variants are loaded, and then the selector checks that first and uses that if its there

hanars · 2023-08-11T19:42:07Z

ui/shared/components/panel/variants/SavedVariants.jsx

-
-    const appliedTagCategoryFilter = tag || (variantGuid ? null : (tableState.categoryFilter || ALL_FILTER))
+    const { familyGuid, variantGuid } = match.params
+    const optionValue = getSelectedTag(tableState.categoryFilter)


notpick, but theres no need to assign this to a variable, it would be more readable if you directly assigned it to the prop

ui/shared/components/panel/variants/selectors.js

…tiple-variant-tag-filtering

ShifaSZ added 6 commits July 20, 2023 11:33

Add multiple variant tags filtering feature.

71e905d

Fix codacy and ALL filter.

424e66b

Merge branch 'dev' into multiple-variant-tag-filtering

cc216dc

Update category and all option controls.

f13dc6c

Update tests.

ab47e9c

Remove unused imports.

f3b94bf

ShifaSZ marked this pull request as ready for review July 27, 2023 13:26

Update reducer to prevent reloading.

48f0111

ShifaSZ commented Jul 27, 2023

View reviewed changes

Correct clear loaded tags.

de8cbd4

ShifaSZ commented Jul 27, 2023

View reviewed changes

ShifaSZ requested a review from hanars July 27, 2023 15:40

hanars reviewed Jul 27, 2023

View reviewed changes

ShifaSZ added 3 commits August 3, 2023 14:33

Use a constant for delimiter and update db filter.

83e3979

Merge branch 'dev' into multiple-variant-tag-filtering

fed9cdd

Update multiple tags DB queries and tests.

ee46678

ShifaSZ commented Aug 3, 2023

View reviewed changes

A better state key generation.

5162ae9

ShifaSZ commented Aug 3, 2023

View reviewed changes

hanars reviewed Aug 4, 2023

View reviewed changes

ShifaSZ requested a review from hanars August 4, 2023 14:34

hanars requested changes Aug 4, 2023

View reviewed changes

ShifaSZ added 3 commits August 4, 2023 15:52

Update the TAG_OPTIONS constant.

561822e

Merge branch 'dev' into multiple-variant-tag-filtering

303981c

Remove project saved-variant multi-tag filtering.

b45b050

ShifaSZ requested a review from hanars August 7, 2023 16:34

hanars requested changes Aug 7, 2023

View reviewed changes

Update the option value and filters props.

6b7a039

ShifaSZ commented Aug 9, 2023

View reviewed changes

ShifaSZ requested a review from hanars August 9, 2023 17:59

hanars requested changes Aug 10, 2023

View reviewed changes

ShifaSZ added 3 commits August 10, 2023 17:18

Save the variants to the state per tag.

7ea4f23

Update the getSelectedTag function.

e46151c

Merge branch 'dev' into multiple-variant-tag-filtering

f05bc63

ShifaSZ commented Aug 10, 2023

View reviewed changes

fix tests.

eacc42c

hanars reviewed Aug 11, 2023

View reviewed changes

ShifaSZ added 2 commits August 11, 2023 14:45

Update the per-tag saved-variant data state.

b2e440e

Add tests for the selector for the summary data.

72711c2

ShifaSZ requested a review from hanars August 11, 2023 19:12

hanars requested changes Aug 11, 2023

View reviewed changes

hanars added 9 commits September 5, 2023 16:26

Merge branch 'dev' of https://github.com/broadinstitute/seqr into mul…

de1927b

…tiple-variant-tag-filtering

clean up selected tag logic

03185c6

Merge branch 'dev' of https://github.com/broadinstitute/seqr into mul…

5072223

…tiple-variant-tag-filtering

update multi tag summary data selector

fae6d42

clean up selectors

664394f

fix tests

fb288fe

diff cleanup

cc04e2c

diff cleanup

00e37d4

better display and behavior

91d3959

hanars requested review from bpblanken and hanars and removed request for hanars September 6, 2023 16:27

bpblanken approved these changes Sep 7, 2023

View reviewed changes

hanars approved these changes Sep 7, 2023

View reviewed changes

hanars merged commit 48143e0 into dev Sep 7, 2023
5 checks passed


		let shownFilters = (discoveryFilters && appliedTagCategoryFilter === DISCOVERY_CATEGORY_NAME) ?
		let shownFilters = (discoveryFilters && firstAppliedTagCategoryFilter === DISCOVERY_CATEGORY_NAME) ?

		dispatch({ type: RECEIVE_DATA, updatesById: responseJson })
		dispatch({ type: RECEIVE_SAVED_VARIANT_TAGS, updates: { [tag]: responseJson.savedVariantsByGuid } })

Multiple variant tags filtering #3502

Multiple variant tags filtering #3502

Conversation

ShifaSZ commented Jul 20, 2023 • edited Loading

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

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

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

hanars Aug 4, 2023 • edited Loading

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

Choose a reason for hiding this comment

ShifaSZ Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

hanars left a 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

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

ShifaSZ Aug 10, 2023 • edited Loading

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

hanars Aug 11, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShifaSZ commented Jul 20, 2023 •

edited

Loading

hanars Aug 4, 2023 •

edited

Loading

ShifaSZ Aug 4, 2023 •

edited

Loading

ShifaSZ Aug 10, 2023 •

edited

Loading

hanars Aug 11, 2023 •

edited

Loading