Clarify log scaled site quality metrics #1624
Conversation
There was a problem hiding this comment.
thank you for adding! I added a couple basic questions.
also quick note: I'm not the best placed to review this code -- perhaps @rileyhgrant or @phildarnowsky could also take a look?
| @@ -625,11 +628,29 @@ const SiteQualityMetricsHistogram = ({ | |||
| }: SiteQualityMetricsHistogramProps) => { | |||
| const isLogScale = metric === 'SiteQuality' || metric === 'AS_QUALapprox' || metric === 'DP' | |||
There was a problem hiding this comment.
SiteQuality, AS_QUALapprox, and DP aren't currently log scaled on the variant pages as far as I can tell (https://gnomad.broadinstitute.org/variant/1-55051215-G-GA?dataset=gnomad_r4) -- should they be?
There was a problem hiding this comment.
Just updated that for the labeling to include the isLogScale values, one question does the graph look correctly scaled?
There was a problem hiding this comment.
thanks! what do you mean for correctly scaled?
There was a problem hiding this comment.
that the graphs are already log scaled and just needed the labeling additions?
There was a problem hiding this comment.
ah thanks for clarifying. it seems like the graphs aren't actually log scaled. for that variant (1-55051215-G-GA), the browser is showing this for the exomes:
the value for AS_QUALapprox in the v4.1 exomes release HT is 262031, and that value is not log transformed
There was a problem hiding this comment.
Using the same variant and view provided this what we have right now after adding the "log()" around the values, would it be better to evaluate or leave it like that with the log labeling?
There was a problem hiding this comment.
I'm a little confused -- the plot doesn't seem to be log scaled in that second screenshot. The x-axis in the first screenshot (untransformed plot) has the same range as the second log-transformed screenshot, which shouldn't be the case.
Also, log10(262031) is 5.418, and it looks like that value is now being labeled at the correct location in the second screenshot with the vertical line and arrow. However, in the untransformed plot, 262031 seems to be near the middle of the distribution, and it should stay at the same spot once the rest of the data are transformed (but it is currently far off to the left).
let me know if this doesn't make sense or if I've completely missed something
|
@ch-kr please note I use @phildarnowsky-broad for work code |
browser/src/VariantPage/__snapshots__/VariantPage.spec.tsx.snap
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Code's looking good, there is just one place where I suggest adding a comment to clarify something.
Also I'd suggest, before you merge this in, to rebase it all down into one commit, to keep the intermediate half-formed versions of the code out of the commit history.
update to use log base 10 update log labels for metric table update jest snapshot clean up function formatMetricValue refactor exome and genome legend text follow up testing draft add just snapshot testing for now fix linting Rename existing getFlagsForContext Expose exome- and genome-specific flags in API Move regional flags (par, lcr, segdup) from exome/genome flags to variant flags in API Incorporate exome/genome flags into frontend finalizing snapshot apply log labeling and value scaling only to AS_VarDP add comment regarding use of isLogScale
elissa-alarmani
force-pushed
the
ea-loglabel-240906
branch
from
c58650a to
368153a
Compare
Resolves #1615
Regarding the site quality metrics histogram, for the metrics
["AS_VarDP", "QUALapprox", "AS_QUALapprox"], here we add the 'log' label to the x-axis and update the displayed value as the calculated log value.