-
Notifications
You must be signed in to change notification settings - Fork 0
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
mrc-5485 Use multiple graph config in Sensitivity plots #212
Conversation
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.
Looks good to me! couple unrelated things i noted while playing around with this:
- i know we moved the Time axis label to the bottom but now with like 3 graphs it feels like the user can be confused about what the x axis is
- now that theres scrolling in the run, sens and stoch tabs, i cant easily switch
Perhaps for the second point we make the tabs sticky and for the first one we also make the time label sticky in some way?
I think in practice people are going to be aware of what the x axis is because it's (almost) always time (and when it isn't we i.e. Sensitivity summary, we label every graph). Sticky tabs is a great idea, I'll make a ticket. |
https://mrc-ide.myjetbrains.com/youtrack/issue/mrc-5583/sticky-tab-headers |
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 killed the browser tab by requesting 100 sensitivity runs but I'm not blaming you for that!
<sensitivity-summary-plot v-else :fade-plot="!!updateMsg"></sensitivity-summary-plot> | ||
<template v-for="(config, index) in graphConfigs" :key="config.id"> | ||
<sensitivity-traces-plot | ||
v-if="tracesPlot" |
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.
Can tracesPlot change during a page visit? If so, then v-show may be more performant than v-if.
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.
Yeah it can, though I think the summary plot is fairly rarely used compared to the traces plot, so I'm inclined to leave that to render as required for now.
This branch adds multiple graphs to the Sensitivity tab, following the same graph configuration as the Run tab. You should see that Sensitivity graphs (both Trace over Time and the summary plots) now display for all graph configs, not just the first one. This adds further code repetition which will be factored out in mrc-5572.
There's a also a bug fix for an issue I noticed, where sometimes deletion of the final graph config did not result in the Time axis label being applied to the new final graph - this only seemed to apply to Stochastic and Sensitivity graphs (not sure why!). The bug occured because the plots are not fully reactive to the store because of needing to manually nudge plotly to redraw, Because of this we have an array of "redraw watches" and a watch in WodinPlot which kicks plotly when it detects a change. For the fix, I have added graph config length to this array.