Constraint for mitochondrial genes#1451
Conversation
|
Resolves: #1409 |
e7f9156 to
6e05a40
Compare
|
Building a demo did fail for me so there will be at least one more fixup to come |
fc452af to
c83f2df
Compare
2b7ac28 to
f2bd9b6
Compare
|
Note to self: need to get copy for both info buttons from Nicole |
rileyhgrant
left a comment
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
a452ba1 to
4d7f00c
Compare
|
@rileyhgrant this latest push should address all your feedback. I came up with a solution to the |
rileyhgrant
left a comment
There was a problem hiding this comment.
Nice, lgtm!
I like the solution on how to neatly keep unclamped start and stop around for rendering tooltips as needed.
4d7f00c to
ed33d6b
Compare
ed33d6b to
309d029
Compare
rileyhgrant
left a comment
There was a problem hiding this comment.
Nice, went through commit by commit and checked out the new fixup commits, as well as poked around on a local browser pointing to a local API -- looking good! Nicely done.
LGTM
|
@rileyhgrant |
|
@rileyhgrant please disregard earlier comment, meant to post that on a different PR |
dff7adb to
074f13e
Compare
8cbd5d4 to
b206cf5
Compare
85f64a7 to
3da7488
Compare
|
@rileyhgrant FYI two small followups to come here in the next hour or so |
5dc57a6 to
9d2cde4
Compare
rileyhgrant
left a comment
There was a problem hiding this comment.
LGTM!
Love all the testing in this, also the very clear fixup commits. Nicely done :)
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.
Here we make a change to some of the markup and CSS around the "Show transcripts" button in the gene page. This will help avoid breaking the page layout when we hide this button for mitochonrial genes in an upcoming commit.
9dd3ff4 to
233ebdc
Compare






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.