Constraint for mitochondrial genes #1451
Conversation
|
Resolves: #1409 |
phildarnowsky-broad
force-pushed
the
mito_constraint
branch
from
e7f9156
to
6e05a40
Compare
|
Building a demo did fail for me so there will be at least one more fixup to come |
phildarnowsky-broad
force-pushed
the
mito_constraint
branch
from
fc452af
to
c83f2df
Compare
phildarnowsky-broad
force-pushed
the
mito_constraint
branch
7 times, most recently
from
2b7ac28
to
f2bd9b6
Compare
|
Note to self: need to get copy for both info buttons from Nicole |
There was a problem hiding this comment.
Nicely done on the whole, one main comment with requested changes about a regression in what the RMC tooltip now displays, and a few minor thoughts. Apart from that lots of great things in here.
The refactoring of the merge regions function in particular is great to see an example of that, it's way more comprehensible now.
Also the work towards unifying the constraint tracks is excellent.
| <dd>{`${region.chrom}:${region.region_start}-${region.region_stop}`}</dd> | ||
| <dd>{`${region.chrom}:${region.start}-${region.stop}`}</dd> |
There was a problem hiding this comment.
region_start and region_stop are, while in hindsight probably poorly named, not redundant. Also in hindsight this may have been a good place for a comment in the code discussing this, since it was a specific request and a bit subtle.
The track renders and colors each exon or portion of an exon based on the start and stop coordinates output from finding the overlap of the two passed in region arrays, but the RMC team wanted the hover over text to reflect the coordinates of the constrained region (region_start and region_stop), not the coordinates of the exon or portion of the exon (start and stop).
The RMC team had reached out to us to change the behavior to be this way, and at that time passing the region_start and region_stop coordinates so that each output overlapping region would have those values was my solution.
Screenshots
In production, since these two exons are part of the same constrained region, the tooltips show the start and stop coordinates for the region:
-
With this change, this behavior has reverted to displaying the coordinates for the block thats rendered:
browser/src/ConstraintTrack.tsx
Outdated
| export const regionsInExons = <R extends GenericRegion>(regions: R[], exons: Exon[]): R[] => { | ||
| const sortedRegions = regions.sort((a, b) => a.start - b.start) | ||
| const sortedExons = exons.sort((a, b) => a.start - b.start) | ||
|
|
There was a problem hiding this comment.
Woohoo, very nicely done.
I honestly avoided touching that function after extending the tests for it since it was a bit unreadable to my eyes at that time, this goes a long way towards making it approachable -- kudos.
There was a problem hiding this comment.
refactoring this bit was very satisfying
| <RegionAttributeList> | ||
| <div> | ||
| <dt>Coordinates:</dt> | ||
| <dd>{`m:${region.start}-${region.stop}`}</dd> |
There was a problem hiding this comment.
This is very minor -- it appears as though regions and variants use a capital M to denote a region in mitochondrial DNA, rather than a lowercase.
https://gnomad.broadinstitute.org/gene/ENSG00000198899?dataset=gnomad_r4
| <td>{expected.toFixed(1)}</td> | ||
| <td>{observed.toFixed(1)}</td> | ||
| <td> | ||
| {oe.toFixed(2)} ({oeLower.toFixed(2)}-{oeUpper.toFixed(2)}) |
There was a problem hiding this comment.
I wonder if we could label this with o/e = ... to be more in line with the non-mito gene constraint table, or since there's only one 'contraint metric' in this cell, just label the column 'Observed/Expected'. It took me a few seconds to understand what that cell was showing without the context of some sort of label.
There are also a few other bells and whistles (that visual indication of o/e) that could be added to be more in line, but this may be getting into more of a question for Matt or KC type territory.
Mito gene constraint table
Non-mito gene constraint table
phildarnowsky-broad
force-pushed
the
mito_constraint
branch
3 times, most recently
from
a452ba1
to
4d7f00c
Compare
|
@rileyhgrant this latest push should address all your feedback. I came up with a solution to the |
phildarnowsky-broad
force-pushed
the
mito_constraint
branch
from
4d7f00c
to
ed33d6b
Compare
We're about to extend this type so it's a good juncture to catch up on some small fixes in this area.
Here we extract a generic `ConstraintTrack` component from `RegionalMissenseConstraintTrack` so that we can build a mitochondrial constraint track on the new generic component.
phildarnowsky-broad
force-pushed
the
mito_constraint
branch
from
ed33d6b
to
309d029
Compare
|
@rileyhgrant |
|
@rileyhgrant please disregard earlier comment, meant to post that on a different PR |
This adds a table and a region plot of constraint for mitochondrial genes. One unusual thing I've done here is to keep this data in memory on the API server, rather than create a data pipeline. There's so little data--just over 100 rows--that a pipeline felt like massive overkill.
Here's a typical track. Note that, per discussion with Nicole, there aren't really different degrees of constrained regions, it's more a binary choice of "is constrained" or "isn't constrained". Hence just the one kind of block in the track.