Skip to content

Populate tooltips with summary data#30

Merged
david-mears-2 merged 27 commits intorename-enumsfrom
populate-tooltips-with-summary-data
Jan 22, 2026
Merged

Populate tooltips with summary data#30
david-mears-2 merged 27 commits intorename-enumsfrom
populate-tooltips-with-summary-data

Conversation

@david-mears-2
Copy link
Copy Markdown
Contributor

@david-mears-2 david-mears-2 commented Jan 16, 2026

(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.

@codecov
Copy link
Copy Markdown

codecov bot commented Jan 16, 2026

Codecov Report

❌ Patch coverage is 98.75000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 97.67%. Comparing base (038a58a) to head (4cbd8e1).
⚠️ Report is 28 commits behind head on rename-enums.

Files with missing lines Patch % Lines
src/composables/usePlotTooltips.ts 96.87% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot AI and others added 8 commits January 16, 2026 19:31
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
@david-mears-2 david-mears-2 marked this pull request as ready for review January 19, 2026 15:51
Copy link
Copy Markdown
Contributor

@EmmaLRussell EmmaLRussell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment on lines +30 to +34
const summaryDataRow = summaryTableData.find(d => {
return Object.entries(dimensions).every(([axis, dim]) => {
return !dim || d[dim] === point.metadata?.[axis as Axis];
});
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a little inscrutable, maybe worth a comment - is this about right?

Suggested change
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];
});
});

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment


const includePositiveSign = ciLower && ciLower < 0;
const ciLowerMaybeWithSign = ciLower?.toFixed(2);
const ciUpperMaybeWithSign = `${includePositiveSign ? `${ciUpper && ciUpper > 0 ? '+' : ''}` : ''}${ciUpper?.toFixed(2)}`;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?).

Suggested change
const ciUpperMaybeWithSign = `${includePositiveSign ? `${ciUpper && ciUpper > 0 ? '+' : ''}` : ''}${ciUpper?.toFixed(2)}`;
const ciUpperSign = (includePositiveSign && ciUpper > 0) ? '+' : '';
const ciUpperMaybeWithSign = `${ciUpperSign}${ciUpper?.toFixed(2)}`;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

@david-mears-2 david-mears-2 Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm removing the 'DataRow' type and just tolerating the slight duplication

lower_bound: -2.434,
upper_bound: -2.422,
});
expect(dataStore.histogramData[0]).toEqual(expect.objectContaining({ disease: "Cholera" }));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
expect(dataStore.histogramData[0]).toEqual(expect.objectContaining({ disease: "Cholera" }));
expect(dataStore.histogramData[0]["disease"]).toEqual("Cholera");

Similarly for line 55

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why I removed those checks, reinstating

@david-mears-2
Copy link
Copy Markdown
Contributor Author

david-mears-2 commented Jan 21, 2026

Weird about the CI numbers not matching. Did the summary table data come from the same report version as the rest?

It still looks like this after updating all data to the latest packet. Katy's looking into it I think.

@david-mears-2
Copy link
Copy Markdown
Contributor Author

david-mears-2 commented Jan 21, 2026

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 have now implemented this.

I think Katy suggested just including mean and 95% CI so not sure if median really needed.

Removed median - better for space too.

@david-mears-2 david-mears-2 merged commit b5c53bf into rename-enums Jan 22, 2026
4 checks passed
@david-mears-2 david-mears-2 deleted the populate-tooltips-with-summary-data branch January 22, 2026 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants