Populate tooltips with summary data#30
Conversation
…tips-with-summary-data
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## rename-enums #30 +/- ##
================================================
+ Coverage 97.64% 97.67% +0.03%
================================================
Files 18 18
Lines 382 430 +48
Branches 93 103 +10
================================================
+ Hits 373 420 +47
- Misses 9 10 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: david-mears-2 <60350599+david-mears-2@users.noreply.github.com>
Co-authored-by: david-mears-2 <60350599+david-mears-2@users.noreply.github.com>
…tips Co-authored-by: david-mears-2 <60350599+david-mears-2@users.noreply.github.com>
…imension Co-authored-by: david-mears-2 <60350599+david-mears-2@users.noreply.github.com>
Add unit tests for tooltip summary data and conditional dimension display
EmmaLRussell
left a comment
There was a problem hiding this comment.
Tooltips look great - I like the colour blob to visually link with the histogram.
Check with Katy on this, but I would expect the number format in the tooltips to match the numbers on the axes - so 0.18 is fine for non-log but would expect it to be 1.8 × 10⁻¹ in log scale.
I think Katy suggested just including mean and 95% CI so not sure if median really needed.
Weird about the CI numbers not matching. Did the summary table data come from the same report version as the rest?
src/composables/usePlotTooltips.ts
Outdated
| const summaryDataRow = summaryTableData.find(d => { | ||
| return Object.entries(dimensions).every(([axis, dim]) => { | ||
| return !dim || d[dim] === point.metadata?.[axis as Axis]; | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This is a little inscrutable, maybe worth a comment - is this about right?
| const summaryDataRow = summaryTableData.find(d => { | |
| return Object.entries(dimensions).every(([axis, dim]) => { | |
| return !dim || d[dim] === point.metadata?.[axis as Axis]; | |
| }); | |
| }); | |
| // Find the summary table row whose values for the current UI row and band | |
| // dimensions (and column, if set) match the values of the tooltip point | |
| const summaryDataRow = summaryTableData.find(d => { | |
| return Object.entries(dimensions).every(([axis, dim]) => { | |
| return !dim || d[dim] === point.metadata?.[axis as Axis]; | |
| }); | |
| }); |
There was a problem hiding this comment.
Added a comment
src/composables/usePlotTooltips.ts
Outdated
|
|
||
| const includePositiveSign = ciLower && ciLower < 0; | ||
| const ciLowerMaybeWithSign = ciLower?.toFixed(2); | ||
| const ciUpperMaybeWithSign = `${includePositiveSign ? `${ciUpper && ciUpper > 0 ? '+' : ''}` : ''}${ciUpper?.toFixed(2)}`; |
There was a problem hiding this comment.
I think this would be more readable as two lines rather than having a bunch of logic in the string format. Don't really need to say ciUpper && ciUpper > 0 because null is not greater than 0 (assuming null is the only falsy option here?).
| const ciUpperMaybeWithSign = `${includePositiveSign ? `${ciUpper && ciUpper > 0 ? '+' : ''}` : ''}${ciUpper?.toFixed(2)}`; | |
| const ciUpperSign = (includePositiveSign && ciUpper > 0) ? '+' : ''; | |
| const ciUpperMaybeWithSign = `${ciUpperSign}${ciUpper?.toFixed(2)}`; |
There was a problem hiding this comment.
I can't do ciUpper && ciUpper > 0 because of typescript but I can do ciUpper! > 0.
src/types.ts
Outdated
| [HistColumn.UPPER_BOUND]: number; | ||
| [HistColumn.COUNTS]: number; | ||
| }; | ||
| export type HistDataRow = DataRow & Record<HistColumn, number>; |
There was a problem hiding this comment.
Why change the previous definition of HistDataRow? That seems clearer to me, and could use same approach to SummaryTableDataRow. It's a bit more verbose but also more readable. Also - this implies that the HistColumn and SummaryTableDataRow may not contain all the expected data value columns - is that actually the case?
There was a problem hiding this comment.
I wanted to put all the columns shared between both types of data file into the DataRow type. Maybe it was premature optimization!
The optional columns:
- activity type may not be present depending on the file
- same for country and subregion
- the location column is added by the data store as a merger of country/subregion/global (so it doesn't exist until processed by the store)
There was a problem hiding this comment.
I'm removing the 'DataRow' type and just tolerating the slight duplication
tests/unit/stores/dataStore.spec.ts
Outdated
| lower_bound: -2.434, | ||
| upper_bound: -2.422, | ||
| }); | ||
| expect(dataStore.histogramData[0]).toEqual(expect.objectContaining({ disease: "Cholera" })); |
There was a problem hiding this comment.
Not sure why you've removed the full equality check here, but if you really do want to just check for disease value, I don't think objectContaining is really doing much here, could just have:
| expect(dataStore.histogramData[0]).toEqual(expect.objectContaining({ disease: "Cholera" })); | |
| expect(dataStore.histogramData[0]["disease"]).toEqual("Cholera"); |
Similarly for line 55
There was a problem hiding this comment.
I'm not sure why I removed those checks, reinstating
It still looks like this after updating all data to the latest packet. Katy's looking into it I think. |
I have now implemented this.
Removed median - better for space too. |
(failed to use the branch name to tag https://mrc-ide.myjetbrains.com/youtrack/issue/VIMC-9196/Populate-tooltip-content-with-data-from-summary-tables )
Changes
dataStore
First there were the changes to the dataStore, which originally just managed histogram data json files, but now does what we call 'summary table' data: that's the mean, median, and 95% CI for each set of ridgeline data.
I'm planning to reuse this same summary data for the ordering / ranking of plot rows, in a later PR.
Tooltips themselves
That was required to get the data to put in tooltips. The composable is responsible for creating a callback (at chart initialization time) which is invoked when the chart is hovered: returns HTML that gets slotted inside a skadi-chart element.
Open problems
An experimental branch visualizing the 95% CI intervals makes me worry that either the summary table data provided to us by VIMC is incorrect, OR I have incorrectly looked up the wrong summary table data in some way:
AI??
I got copilot to write some basic unit tests in another PR, which I reviewed (gave feedback on, finally submitted another human-written commit on top), and then merged into this branch.