Volcano #19
Volcano #19
Conversation
|
One question - i'm starting to change my mind about who does the color choices. Maybe plot.data only sends back data and the component breaks up into colors. Seems like this volcano component could be a little smarter and not rely so much on everyone else... |
|
All up and running now! Ready for review! |
| import { NumberRange } from '../types/general'; | ||
| import { | ||
| XYChart, | ||
| Tooltip, |
There was a problem hiding this comment.
nit: 'Tooltip' is declared but its value is never read.
| // Determine mins, maxes of axes in the plot. | ||
| // These are different than the data mins/maxes because | ||
| // of the log transform and the little bit of padding. | ||
| // |
| // From docs " For correct tooltip positioning, it is important to wrap your | ||
| // component in an element (e.g., div) with relative positioning." |
There was a problem hiding this comment.
random: If you or anyone would like to understand absolute/relative positioning, this Codepen might help.
The docs say this because it sounds like the tooltips are positioned absolutely. Absolutely positioned elements position themselves in relation to their nearest parent that's positioned relatively. 🤯
You likely know this, but I just wanted to put it in there in case!
There was a problem hiding this comment.
i did not know this! Thank you! That was a really clear example
| yValue: number, // the raw pvalue | ||
| significanceThreshold: number, | ||
| log2FoldChangeThreshold: number, | ||
| significanceColors: string[] // Assuming the order is [high, low, not significant] |
There was a problem hiding this comment.
suggestion: This isn't at all unclear code or anything complicated, so this is really just a suggestion. I don't know if it makes the code better. Given this assumption, you could name the indicies:
const HIGH = 0;
const LOW = 1;
const INSIGNIFICANT = 2;
if (yValue >= significanceThreshold) {
return significanceColors[INSIGNIFICANT];
}
if (xValue >= log2FoldChangeThreshold) {
return significanceColors[HIGH];
}
...
| if (dataXMin && dataXMax) { | ||
| xMin = Math.log2(dataXMin); | ||
| xMax = Math.log2(dataXMax); | ||
| xMin = xMin - (xMax - xMin) * 0.05; |
There was a problem hiding this comment.
nit/suggestion: 0.05 is the measure of statistical significance, right? Maybe we can save that to a constant called STATISTICAL_SIGNIFICANCE_THRESHOLD?
|
@dmfalke and @adnauseum ready for your review again! I didn't end up making the find-min-and-max part into a function since we were sort of back and forth about it and it's a little unique to volcano. If you feel strongly about it though i'm happy to do it! One new change - we learned that we don't expect to ever need the raw fold change values, so we're just going to have the backend return log2(fold change) so that the frontend doesn't have to do it. I'll turn to working on the confluence doc for a bit while it's all fresh in my mind. |
| plugins: ['@babel/plugin-proposal-nullish-coalescing-operator'], | ||
| plugins: [ | ||
| '@babel/plugin-proposal-nullish-coalescing-operator', | ||
| '@babel/plugin-syntax-class-properties', |
There was a problem hiding this comment.
Why was this added? I don't see class properties used anywhere.
There was a problem hiding this comment.
Thank you!! I forgot to ask about this in our PR review! I was curious since it's scoped to storybook.
Ann's probably hacking us 🤔
There was a problem hiding this comment.
ahh yes good catch! This was a byproduct of my adventures trying to install some of the visx packages a while back. I just removed it and all seems to work 👍
| @@ -283,6 +283,9 @@ export const gradientConvergingColorscaleMap = scaleLinear<string>() | |||
| .range(ConvergingGradientColorscale) | |||
| .interpolate(interpolateLab); | |||
|
|
|||
| // Significance colors | |||
| export const significanceColors = ['#AC3B4E', '#0E8FAB', '#B5B8B4']; | |||
There was a problem hiding this comment.
This is the only thing I saw that made me a bit uncomfortable.. say some instances of volcano plot want to use two colors rather than three, and not distinguish high vs low?
There was a problem hiding this comment.
I swapped the order around so it goes 'not significant, high, low". That way when we do have a use case for two colors only, the component can get a prop that says use 2 or 3 colors, and it'll just use the first two or first three colors from the list. If for some reason we wanted four colors later, we could just add onto this array.
This is assuming that we're happy with the colors being repeated in this way. Worth a discussion when that comes around i'd imagine.
I also am not considering these choices of colors final. I think they should go through data viz first. I'll make a new issue to be sure it doesn't get lost!
|
Thanks everyone for the feedback!! |
Resolves VEuPathDB/web-components#463
Required for first delivery of mbio differential abundance app. More details on the plot here
This PR adds a volcano plot component using visx. It does not attempt to add everything necessary for the "fully functional" volcano plot that lives in the EDA, but gives the backbones for us to build upon in the future.
To Dos left for later work
Other notes: