-
Notifications
You must be signed in to change notification settings - Fork 0
Add ridgeline chart #17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17 +/- ##
==========================================
+ Coverage 94.07% 97.18% +3.10%
==========================================
Files 9 14 +5
Lines 135 284 +149
Branches 41 81 +40
==========================================
+ Hits 127 276 +149
Misses 8 8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
EmmaLRussell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to get the chart in! It is quite confusing without the legend and tooltips so it's good they're on the way! Colors look good.
It feels like there's a bit of overloading of "x" and "y" in the RidgelinePlot component. Sometimes you're dealing with x,y points which make up the outline of the histogram "within band". Other times "x" and "y" refer to the columns and rows at the higher level. I wonder if it would be clearer to use "row" and "column" to refer to the latter. Or at least to separate out the histogram drawing parts into a separate module.
src/components/RidgelinePlot.vue
Outdated
| <div | ||
| ref="chartWrapper" | ||
| id="chartWrapper" | ||
| v-if="ridgeLines.length" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could do this as a v-else
src/stores/colorStore.ts
Outdated
| return { | ||
| 2: [ibmColors.purple70, ibmColors.teal50], // IBM 2-color group option 1 | ||
| 3: [ibmColors.purple70, ibmColors.cyan50, ibmColors.magenta50], // IBM 3-color group option 4 (in reverse order to put purple70 first) | ||
| 4: [ibmColors.purple70, ibmColors.cyan90, ibmColors.teal50, ibmColors.magenta50], // IBM 4-color group option 2 | ||
| 5: [ibmColors.purple70, ibmColors.cyan50, ibmColors.teal70, ibmColors.magenta70, ibmColors.red90], // IBM 5-color group option 1 | ||
| }[categories.value.length] ?? Object.values(ibmColors); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This definition of different colour sets with different counts could just be a const, with the selection from it being done in the computed.
| <p v-if="dataStore.fetchErrors.length" class="mt-auto"> | ||
| {{ dataStore.fetchErrors.map(error => error.message).join(', ') }} | ||
| </p> | ||
| <!-- Legend for manual testing only --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tbh, I'd just include this for now, it's very useful!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't want to make the PR even bigger, nor hold up its publication by requiring this legend to be tested - will do this in a sub-branch.
src/components/RidgelinePlot.vue
Outdated
| // Construct histogram/ridge-shaped lines by building area lines whose points trace the | ||
| // outline of the histogram bars (including the spaces in between them). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like it might be something to include in skadi charts itself eventually.. While it's here, I wonder if it would be worth separating out the bits that know how to draw a histogram (maybe as a composable) from the bits that understand what's in the store.
src/components/RidgelinePlot.vue
Outdated
| // closingOffPoint is a point at y=0 to close off the histogram bar, which will be needed if either: | ||
| // 1) this ends up being the last bar in the line, or | ||
| // 2) there is a data gap until the next bar in this line. | ||
| const closingOffPoint = { x: dataRow[HistCols.UPPER_BOUND], y: 0 }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to return this for every bar, if it's usually not going to be used? Caller can always work it out as {x: topRightPoint, y:0} cant' they..?
UPDATE, oh I see. I've just read constructLines and now understand that the whole trio of points are always appended to the list. Might be worth indicating here that this is the plan.
src/components/RidgelinePlot.vue
Outdated
| }; | ||
| const shouldDisplayPlotRow = (lines: Record<string, LineConfig<LineMetadata>>): boolean => { | ||
| if (appStore.dimensions[Axes.Y] !== Dimensions.DISEASE || appStore.dimensions[Axes.WITHIN_BAND] !== Dimensions.LOCATION) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An explanation for this bit would be nice too.
src/components/RidgelinePlot.vue
Outdated
| lines[xCat]![yCat]![withinBandCat] = newLine; | ||
| } else { | ||
| // A line already exists for this combination of categorical axis values, so we can append some points to it. | ||
| const previousPoint = line.points[line.points.length - 1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can rely on these to be in the right order?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could add a validateJsonData node script that verifies this is the case. Or, a much more lightweight option, throw an error here if previousPoint.x > lowerBound.
There is a line in the report that we're relying on to sort everything:
https://github.com/vimc/montagu-reports/blob/2dac0dc6d286c260f46c919e6ad88893a9d8962e/src/paper-four-figures/R/data_viz_processing_funs.R#L35C5-L35C25
arrange(lower_bound)
src/components/RidgelinePlot.vue
Outdated
| // we should remove the previous close-off point, as we are continuing the line directly from the previous bar to this bar. | ||
| line.points.pop(); | ||
| } else if (previousPoint) { | ||
| line.points.push({ x: lowerBound, y: 0 }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| line.points.push({ x: lowerBound, y: 0 }); | |
| // There is a previous section, complete with close -off point but this bar is disconnected from it. | |
| // Leave the previous close-off point and make a new starting point at y=0 | |
| line.points.push({ x: lowerBound, y: 0 }); |
| "label": "MenA" | ||
| }, | ||
| { | ||
| "value": "MenACWYX", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's MenACWX?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comes out of the dataviz csv's. Seems to be a vaccine for meningitis, according to Google, but we're supposed to be plotting diseases, not vaccines, so I don't know why it's here. It likely derives ultimately from https://github.com/vimc/montagu-reports/blob/2dac0dc6d286c260f46c919e6ad88893a9d8962e/shared/metadata/disease_vaccine.csv#L29 via a chain of various packets.
I will raise this with Katy in a future demo to see if it's surprising!
src/stores/colorStore.ts
Outdated
| // If there are more categories than pre-defined colors, recycle colors traversing the list backwards, | ||
| // since the palettes are designed to maximize contrast between neighboring colors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't really happen though, should it? We don't want to end up with duplicate colours at all. It would be worth showing a warning on the page if that happens so we can spot it and add a more numerous set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible in theory because the length of diseaseOptions.json is 16 and I can't guarantee that no permutation of selections will ever use all diseases.
IBM colors was the longest set of accessibility-friendly colors that I could find online. I will merge in 2 other colors with it to make up the total to 16. I'm not sure how to optimise for accessibility (without putting in a large amount of work - I couldn't find any source in github or elsewhere for how these 14 were selected) so I will approximate that by maximizing light/dark contrast: I will add black and white to take us from 14 to 16.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've refactored the color store to stop assigning colors up front when the app store filters change, but instead to assign them as needed (whenever a line with a new category is initialized). This ensures that we don't 'waste' colors by assigning them to categories that, although included in the app store filter, are never displayed due to not having data.
…exist basis Given shouldDisplayPlotRow filters some rows from being displayed, we need to ensure that we only draw from the accessible palette for rows that will be displayed, otherwise we break the assumption that the colors are in the correct order (which was selected to maximize accessibility) and we draw colors from later in the palette unnecessarily.
ridgeLines is now a computed instead of a ref updated by a watcher styles (colors) are handled by the RidgelinePlot component since those are a configuration detail (useHistogramLines could be thought of as akin to a layer for skadi-chart that we may in fact one day put into skadi-chart)
66a171b to
eb9c05e
Compare
I've now renamed references to 'x categorical' and 'y categorical' throughout the app to 'column' and 'row', so that x and y refer only to numerical things. |
1000 of the diff is package-lock.json.
It's what you've all been waiting for: the plot itself. The appearance of a histogram is created by turning our data, which defines bins in terms of lower and upper bounds (and the count for the bin), into points that define an area-line in the shape of a histogram.
There are still a few tweaks to its display to be worked out on the skadi-chart side, such as margin spacing and tooltips.
Some combinations of options, such as disease MenA and no-split-by-activity-type, do not have any data; for those cases we show a message explaining the lack of data.
I had to update part of the pre-build step to process some more tables into options, as I found some more disease options it had't been picking up.
This branch includes the assigning of colors to categories, as otherwise it's very hard to see whether the graph is showing the right (number of) lines. Color logic is handled by a separate store,
colorStore, to ensure that color assignment is the same across the app (including in a future legend component), and to ensure that the color for 'global' is always the same (if it changes randomly when the selections are updated, I find it confusing).