Add v4 CNVs #1176

elissa-alarmani · 2023-09-14T15:06:29Z

No description provided.

rileyhgrant

I didn't look through every commit, as I believe Phil is working on that, but I did want to chime in and say I noticed that on the demo, searching anything causes a crash of the page.

I'm unsure if there's some relatively quick way to disable the search feature for the v4_cnvs dataset only -- that could be a straightforward way to get the MVP out, and we could just file an issue on that and tackle it later.

Also, the amount of SVs is generally so small for a given gene, that I doubt anyone would really use the search feature.

Edit for clarity, screenshot below. Searching of a CNV variant table causes the page to crash.

mattsolo1 · 2023-10-29T18:55:14Z

Thanks RIley -- we def need to fix search.

phildarnowsky-broad · 2023-10-29T20:33:46Z

I just tried search in that demo and it seemed OK to me, did you fix that already maybe Elissa? Or if not, Riley, could you be a little more specific about what you did?

…

On Sun, Oct 29, 2023 at 2:55 PM Matthew Solomonson ***@***.***> wrote: Thanks RIley -- we def need to fix search. — Reply to this email directly, view it on GitHub <#1176 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AZQ2NNPHRM2HGUK6BKLK2TLYB2RB3AVCNFSM6AAAAAA4YIS5X2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOBUGE4TMNZYHA> . You are receiving this because your review was requested.Message ID: ***@***.***>

elissa-alarmani · 2023-10-29T20:36:47Z

I didn't change the demo yet, but I assumed the issue was that the coverage wasn't loading. I can access gene pages fine through the search but I am starting to notice the coverage isn't appearing when you search for genes vs go to the direct page

rileyhgrant · 2023-10-29T21:26:18Z

Sorry, I wasn't clear. I mean the search on the variant table:

The console in the demo does give a helpful error message with some line numbers.

Possibly not a huge deal, as I doubt many people are going to search a table with a handful of CNVs anyways, but I wanted to flag it cause it does cause a hard crash.

phildarnowsky-broad · 2023-10-29T21:29:05Z

I see the error "e.toLowerCase is not a function", haven't looked at the code but $20 says it's an `undefined` or `null` slipping in there.

…

On Sun, Oct 29, 2023 at 5:26 PM Riley Grant ***@***.***> wrote: Sorry, I wasn't clear. I mean the search on the variant table: [image: Screenshot 2023-10-29 at 16 24 04] <https://user-images.githubusercontent.com/59549713/278898915-42ddefc7-1df8-426d-9de6-6b51229981b1.jpg> The console in the demo does give a helpful error message with some line numbers. — Reply to this email directly, view it on GitHub <#1176 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AZQ2NNK7M3ANZI26E3IJV5LYB3CYLAVCNFSM6AAAAAA4YIS5X2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOBUGIZDSNBYGY> . You are receiving this because your review was requested.Message ID: ***@***.***>

elissa-alarmani · 2023-10-29T23:11:50Z

Thanks for pointing that out Riley! Fixed it and it was a small bug with the getSearchTerms not returning strings

phildarnowsky-broad

I requested quite a few changes, most of which involve cleaning the commit history to make it clearer what's going on. It's probably best if we get together in the morning and walk through the first few rebases together. After that, I'll need to give this at least one more look.

phildarnowsky-broad · 2023-10-29T18:45:35Z

v4-cnvs-test1.ipynb

@@ -0,0 +1,558 @@
+{


There's a couple of changes I'm going to request to this PR that all fall into the category of making the commit history easier to follow. This is beneficial both in the short term as I review this PR now, and in the long term when we have to go back and work on this page later. Apart from that I'll give this general review both before and after you make those changes.

The ideal when you're merging a PR is that, if somebody is reading your commits one after the other, it should look basically like you got all the code right the first time, making a series of commits at logical points as you complete individual parts of what you're working on. Of course this is a lie and everyone reading the log knows it, but it does make the code much easier to understand for the reviewer or maintainer.

The main tool for neatening your commit history is interactive rebasing via git rebase -i. This can be kind of a subtle process so I'm going to start out by giving pretty explicit instructions about what I'm suggesting, but you'll get the idea quickly no doubt.

All that is general context, specific advice to follow.

v4-cnvs-test1.ipynb

data-pipeline/src/data_pipeline/datasets/gnomad_v4/gnomad_v4_cnvs.py

phildarnowsky-broad · 2023-10-29T21:42:30Z

data-pipeline/src/data_pipeline/pipelines/export_to_elasticsearch.py

+            "block_size": 10_000,
+        },
+    },
+    "gnomad_v4_cnv_del_burden": {


This index, as well as dup_burden, don't appear to be used. They're not exposed by the GraphQL API, nor are they inputs to other pipelines. If they're not in fact used by anything, we shouldn't have the pipelines to build or export this data either.

graphql-api/src/graphql/resolvers/copy-number-variants.ts

phildarnowsky-broad · 2023-10-29T23:15:18Z

browser/src/CopyNumberVariantList/CopyNumberVariantPlot.tsx

+        strokeWidth={1}
+      />
+      (
+        <React.Fragment>


FYI <> is equivalent to <React.Fragment>

phildarnowsky-broad · 2023-10-29T23:16:24Z

browser/src/CopyNumberVariantList/CopyNumberVariantPropType.ts

@@ -0,0 +1,61 @@
+import PropTypes from 'prop-types'


We don't use proptypes in new code. But leave it for now.

phildarnowsky-broad · 2023-10-29T23:19:21Z

browser/src/CopyNumberVariantList/CopyNumberVariants.tsx

+  )
+
+  // @ts-expect-error TS(7031) FIXME: Binding element 'scrollOffset' implicitly has an '... Remove this comment to see the full error message
+  const onScrollTable = useCallback(({ scrollOffset, scrollUpdateWasRequested }) => {


As a rule, don't name callback functions for the callback they're attached to. Rather, use the name to describe what they do, as you would with any other function. For instance, here you might call the function scrollToCurrentTrack.

browser/src/CopyNumberVariantList/CopyNumberVariantFilterControls.tsx

phildarnowsky-broad · 2023-10-29T23:23:53Z

browser/src/CopyNumberVariantPage/__snapshots__/CopyNumberVariantPage.spec.tsx.snap

@@ -33,7 +33,7 @@ exports[`CopyNumberVariantPage with dataset gnomad_cnv_r4 with variant of type D
        >
          <a
            className="DatasetSelector__TopLevelNavigationLink-sc-1p2fxbn-1-Link eggTrD"
-            data-item="current_sv_or_cnv_dataset"
+            data-item="current_sv_dataset"


~~I'm not clear on what this commit is about either.~~ nvm, it was late and I was tired and now it's pretty clear.

mattsolo1 · 2023-10-29T23:32:21Z

@phildarnowsky-broad in the interest of time, should we opt for merging this soon and you guys can review the PR as a retrospective later this week? I sort of assured Elissa not to worry too much about commits so that we can get the feature done.

phildarnowsky-broad · 2023-10-30T14:55:33Z

@elissa-alarmani as discussed, due to time pressure, I'm going to fork this branch and prepare a new one with the rebases I mention applied. Once the rush is done we can work through this one together, rebasing is an extremely valuable thing to have in your toolbox.

phildarnowsky-broad · 2023-10-30T16:29:18Z

browser/src/CopyNumberVariantList/CopyNumberVariantFilterControls.tsx

+  }
+`
+
+const CheckboxWrapper = styled.div`


For future reference if you find yourself wanting to write a styled component with no styles, it's probably fine to just use the ordinary HTML element in its place, e.g. <div> here. Don't worry about it for now though.

phildarnowsky-broad · 2023-10-30T16:40:56Z

browser/src/DatasetSelector.tsx

            childReferenceGenome: referenceGenome('gnomad_cnv_r4'),
          },
        ],
-      }
+      },


~~this commented-out chunk can come out~~ and so it does, in a later commit. This spot is a good example of the value of tidying your commit history via rebase, since I spent a little time here reviewing a piece of code that later was cut, meaning that time was wasted.

phildarnowsky-broad · 2023-10-30T16:42:37Z

browser/src/GenePage/CopyNumberVariantsInGene.tsx

+      }
+    }
+  `
+  const url = datasetId === 'gnomad_cnv_r4' ? '/api' : 'https://gnomad.broadinstitute.org/api/'


Don't forget to remove this bit before merging. When I have to do something like this which is purely to rig something temporarily for a demo, I often do that in a separate commit prefixed with the label DONTMERGE, that way I'll see it and remember to drop that commit before I merge to main

phildarnowsky-broad · 2023-10-30T16:44:18Z

browser/src/CopyNumberVariantList/CopyNumberVariants.tsx

@@ -0,0 +1,309 @@
+import React, { useCallback, useMemo, useRef, useState } from 'react'


Pretty sure I see a lot of copypasta in this file but given the circumstances I'm inclined to turn a blind eye this time. Maybe let's you and I look more carefully at this in the near future after the crunch is done.

mattsolo1 · 2023-10-30T19:21:51Z

Closed after rebase in #1225

elissa-alarmani linked an issue Sep 21, 2023 that may be closed by this pull request

Add gnomAD v4 CNVs #1133

Open

11 tasks

elissa-alarmani self-assigned this Sep 21, 2023

elissa-alarmani marked this pull request as draft October 10, 2023 13:37

elissa-alarmani changed the title ~~CNVs Draft~~ Add v4 CNVs Oct 10, 2023

elissa-alarmani marked this pull request as ready for review October 26, 2023 21:55

elissa-alarmani added 25 commits October 29, 2023 11:04

cnv notebook draft

5e65538

inital pipelines and task creation

46ad865

create cnv resolvers

d0103d8

create cnv queries

0110036

create cnv graphql types

a9adba5

create cnvs pipeline and tasks

2d3959a

add cnv dataset as an enum dataset-id

549d1fe

add cnv to gene type

12ad7ad

add cnvs to query type

eef8cfc

add cnvs to region type

a1f257a

add cnv resolvers to schema

72853ba

update singular and plural naming

282385e

python black formatter

197ab86

wip resolvers and queries

52dad61

minor pipeline edits

36dae00

add CNVDatasetId to Gene, Query, Region types

cfcf644

fix resolver and query errors

affe4bb

add cnvs to dataset

526191b

add textOrMissingTextWarning

57a511e

add gnomad_cnv_r4 to search dataset

634c724

add gnomad_cnv_r4 to metadata

26f85b6

create cnv populations table

f8d54b8

add cnv page to variant page router

bdd1fa1

WIP cnv page

094ad47

WIP cnv attribute list

5d72c96

elissa-alarmani added 5 commits October 29, 2023 11:29

remove inital coverage files

b1fabcd

coverage edits

004ba1e

Add snapshot test updates

84fea62

add page changes

4dc9495

add formatting

2b16f07

elissa-alarmani force-pushed the ea/cnvs branch from 9f1c515 to 2b16f07 Compare October 29, 2023 15:48

elissa-alarmani added 4 commits October 29, 2023 12:36

update dataset selection

f709675

remove console.log

9aa35bc

finalize testing files

407263b

add new vcf file path

aa51e93

elissa-alarmani requested review from rileyhgrant, phildarnowsky-broad and mattsolo1 October 29, 2023 17:46

rileyhgrant reviewed Oct 29, 2023

View reviewed changes

elissa-alarmani added 2 commits October 29, 2023 18:37

add chromosome to variant table position

6b5a687

fix variant table broke search

c4180e2

eslint fix

dfff177

phildarnowsky-broad requested changes Oct 29, 2023

View reviewed changes

phildarnowsky-broad requested changes Oct 30, 2023

View reviewed changes

mattsolo1 mentioned this pull request Oct 30, 2023

Add V4 CNVs (after rebasing) #1225

Merged

mattsolo1 closed this Oct 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add v4 CNVs #1176

Add v4 CNVs #1176

elissa-alarmani commented Sep 14, 2023

rileyhgrant left a comment •

edited

Loading

mattsolo1 commented Oct 29, 2023

phildarnowsky-broad commented Oct 29, 2023 via email

elissa-alarmani commented Oct 29, 2023

rileyhgrant commented Oct 29, 2023 •

edited

Loading

phildarnowsky-broad commented Oct 29, 2023 via email

elissa-alarmani commented Oct 29, 2023

phildarnowsky-broad left a comment

phildarnowsky-broad Oct 29, 2023

phildarnowsky-broad Oct 29, 2023

phildarnowsky-broad Oct 29, 2023

phildarnowsky-broad Oct 29, 2023 •

edited

Loading

phildarnowsky-broad Oct 29, 2023

phildarnowsky-broad Oct 29, 2023 •

edited

Loading

mattsolo1 commented Oct 29, 2023

phildarnowsky-broad commented Oct 30, 2023

phildarnowsky-broad Oct 30, 2023

phildarnowsky-broad Oct 30, 2023

phildarnowsky-broad Oct 30, 2023

phildarnowsky-broad Oct 30, 2023

mattsolo1 commented Oct 30, 2023

		@@ -0,0 +1,309 @@
		import React, { useCallback, useMemo, useRef, useState } from 'react'

Add v4 CNVs #1176

Add v4 CNVs #1176

Conversation

elissa-alarmani commented Sep 14, 2023

rileyhgrant left a comment • edited Loading

Choose a reason for hiding this comment

mattsolo1 commented Oct 29, 2023

phildarnowsky-broad commented Oct 29, 2023 via email

elissa-alarmani commented Oct 29, 2023

rileyhgrant commented Oct 29, 2023 • edited Loading

phildarnowsky-broad commented Oct 29, 2023 via email

elissa-alarmani commented Oct 29, 2023

phildarnowsky-broad 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

phildarnowsky-broad Oct 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phildarnowsky-broad Oct 29, 2023 • edited Loading

Choose a reason for hiding this comment

mattsolo1 commented Oct 29, 2023

phildarnowsky-broad commented Oct 30, 2023

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

mattsolo1 commented Oct 30, 2023

rileyhgrant left a comment •

edited

Loading

rileyhgrant commented Oct 29, 2023 •

edited

Loading

phildarnowsky-broad Oct 29, 2023 •

edited

Loading

phildarnowsky-broad Oct 29, 2023 •

edited

Loading