mrc-5442 Per-graph settings, with separate fit plot settings #210

EmmaLRussell · 2024-07-09T15:37:59Z

This branch implements per-graph settings (Y axis log scale and lock), so that the multiple graphs no longer share settings. Graph settings are now a property of each graph config, rather than directly the graphs module. Because the fit graph does not use the multiple graph config, it also has its own graph settings.

In the UI, graph settings are included in the graph config area, which has been slightly restyled - the 'Graph N' headings have been removed and replaced with a border around each config to separate them. The Fit Graph settings are visible on the options tab when Fit tab is open - in this case, the multiple graph configs are hidden too.

One other change is that the plots now take graphConfig as a prop rather than just the graph index - I was having a reactivity problem when graphs were deleted such that the last plot was still trying to read settings from the previous maximum indexed config in the store. Providing config as a prop solved this issue..

I haven't updated all the components to use setup script tag and defineProps, as I did in the last branch for RunPlot - didn't want to add too much additional noise. Could maybe do that update in a separate ticket,

mrc-5440 Store changes preliminary to multiple graphs

david-mears-2

My comments are quite nitty, sorry, I'm prepared for most to be ignored.

I couldn't work out within 1 minute what the 'lock y axis' feature was intended to do. From its name, I expected it to prevent the y-axis automatically adjusting to fit the lines in. But I couldn't make the y-axis automatically adjust, so I don't know what 'locking' is meant to do. I was using the http://localhost:3000/apps/day1/ example and dragging variables back and forth between graphs hoping to see some automatic re-scaling of the y-axis that I could turn off by clicking 'lock'.

david-mears-2 · 2024-07-25T11:32:39Z

app/static/src/app/components/GraphSettings.vue

+        };
+    }
+});
+</script>


It occurs to me that the GraphSettings component checks props.fitPlot in three places to do slightly different computed behavior. So an alternative refactor would be to define the fit plot graph settings as a component that inherits (extends) from the GraphSettings, and overwrites these three computeds. That idea doesn't provide much at the moment, probably, but I thought I'd mention it.

Yeah, could do. I don't think one usage is any more fundamental than the other though. Inclined to leave it as is, slightly messy but simple.

david-mears-2 · 2024-07-25T11:34:38Z

app/static/src/app/components/GraphSettings.vue

+        graphIndex: {
+            type: Number,
+            required: false,
+            default: -1


What's the value in giving the graphIndex a default index of -1? If the prop isn't set, you might get multiple components claiming to have this index.

If you don't provide the prop it should be because fitPlot is true. We could explicitly make this nullable and make null the default, but that's going to clutter up the code with null checks. Or we could squash both props into one and assume fitPlot if index is -1. But I think it's better to be explicit and it's probably ok to assume polite usage - but I'll add an explanatory comment.

Understood, I hadn't thought about the null checks.

david-mears-2 · 2024-07-25T12:49:13Z

app/static/src/app/components/run/RunStochasticPlot.vue

@@ -6,6 +6,9 @@
        :plot-data="allPlotData"
        :redrawWatches="solution ? [solution] : []"
        :linked-x-axis="linkedXAxis"
+        :fit-plot="false"
+        :graph-index="graphIndex"
+        :graph-config="graphConfig"


I suppose the alternative to prop-drilling the graphConfig of each graph would be to pass down only the graphIndex, and have the lowest-level component (WodinPlot) look up the config directly from store.state.graphs.config[props.graphIndex]. Again I don't particularly favour doing that -- we're only drilling through one intermediate component -- it's just an idea that occurs.

We did have that. but as noted in the description, this caused reactivity issues:

One other change is that the plots now take graphConfig as a prop rather than just the graph index - I was having a reactivity problem when graphs were deleted such that the last plot was still trying to read settings from the previous maximum indexed config in the store. Providing config as a prop solved this issue..

I'll actually put a comment in the code to this effect so we remember why we've done it this way.

david-mears-2 · 2024-07-25T12:52:26Z

app/static/src/app/components/run/RunStochasticPlot.vue

@@ -29,6 +33,10 @@ export default defineComponent({
            type: Number,
            default: 0


Note the default graphIndex for this component and for RunPlot, 0, is different from the default graphIndex of GraphSettings and WodinPlot (which is -1), and it's not obvious to me as an outsider to the codebase why these are different.

It makes a sort of sense because RunPlot and RunStochasticPlot will always have fitPlot=false, so should have a valid index. But I think it would make even more sense to make this a required prop!

david-mears-2 · 2024-07-25T12:56:56Z

app/static/src/app/components/sensitivity/SensitivitySummaryPlot.vue

@@ -54,7 +54,8 @@ export default defineComponent({

        const yAxisSettings = computed(() => {
            const isNotTimePlot = plotSettings.value.plotType !== SensitivityPlotType.TimeAtExtreme;
-            const logScale = store.state.graphs.settings.logScaleYAxis && isNotTimePlot;
+
+            const logScale = store.state.graphs.config[0].settings.logScaleYAxis && isNotTimePlot;


So a sensitivity summary plot is guaranteed to correspond to the first graph config?

If not, perhaps the S. S. plot should be given a graphIndex to hold onto and use that to look up the right graphConfig.

It does for now - we're going to implement multiple sensitivity plots in another ticket: https://mrc-ide.myjetbrains.com/youtrack/issue/mrc-5485/Include-all-Graphs-on-Sensitivity-tab

david-mears-2 · 2024-07-25T13:04:04Z

app/static/src/app/components/sensitivity/SensitivityTracesPlot.vue

@@ -48,6 +51,7 @@ export default defineComponent({
        const store = useStore();

        const solutions = computed(() => store.state.sensitivity.result?.batch?.solutions || []);
+        const graphConfig = computed(() => store.state.graphs.config[0]);


Same question - unclear to me why we know that the relevant graph config is the first one.
We should do something that ensures that the 0 of line 13 and the 0 of this line are always equal: e.g. move this computed into the child so that the graphIndex prop is used to look up the graphConfig, or set a graphIndex variable in this component that both line 13 and this line can reference.

Again, we'll sort this in mrc-5485, it can just look at the first config until then.

david-mears-2 · 2024-07-25T13:23:06Z

app/static/src/app/store/graphs/state.ts

+    logScaleYAxis: false,
+    lockYAxis: false,
+    yAxisRange: [0, 0]
+});


For newcomers to the codebase, it would be extremely helpful to have commentary in this file that explains the difference between "graph settings", "graph config", and "graphs state" -- the names are roughly synonymous.

Excellent point, will add some comments

david-mears-2 · 2024-07-25T13:36:03Z

Role-playing as a QA tester. Do we want to set limits on what people can do their scales?

EmmaLRussell

couldn't work out within 1 minute what the 'lock y axis' feature was intended to do. From its name, I expected it to prevent the y-axis automatically adjusting to fit the lines in. But I couldn't make the y-axis automatically adjust, so I don't know what 'locking' is meant to do. I was using the http://localhost:3000/apps/day1/ example and dragging variables back and forth between graphs hoping to see some automatic re-scaling of the y-axis that I could turn off by clicking 'lock'.

This works for me:

Add a second graph
Drag "I" to the second graph - The graph auto-scales to I's maximum value, ~160K
Check "lock y axis" for the second graph
Drag "R" to the second graph. The y axis is locked and does not autoscale to R's maximum value (800K)
Uncheck "lock y axis" for the second graph - the second graph autoscales to R's max value.

You can also play around with adjusting the axes by dragging on the graph area to select a portion of the graph. Again, you should find that the y axis of this selection if fixed if you subsequenly lock y axis.

EmmaLRussell · 2024-07-25T14:25:14Z

app/static/src/app/components/GraphSettings.vue

+        graphIndex: {
+            type: Number,
+            required: false,
+            default: -1


If you don't provide the prop it should be because fitPlot is true. We could explicitly make this nullable and make null the default, but that's going to clutter up the code with null checks. Or we could squash both props into one and assume fitPlot if index is -1. But I think it's better to be explicit and it's probably ok to assume polite usage - but I'll add an explanatory comment.

EmmaLRussell · 2024-07-25T14:29:44Z

app/static/src/app/components/GraphSettings.vue

+        };
+    }
+});
+</script>


Yeah, could do. I don't think one usage is any more fundamental than the other though. Inclined to leave it as is, slightly messy but simple.

EmmaLRussell · 2024-07-25T14:31:53Z

app/static/src/app/components/run/RunStochasticPlot.vue

@@ -6,6 +6,9 @@
        :plot-data="allPlotData"
        :redrawWatches="solution ? [solution] : []"
        :linked-x-axis="linkedXAxis"
+        :fit-plot="false"
+        :graph-index="graphIndex"
+        :graph-config="graphConfig"


We did have that. but as noted in the description, this caused reactivity issues:

One other change is that the plots now take graphConfig as a prop rather than just the graph index - I was having a reactivity problem when graphs were deleted such that the last plot was still trying to read settings from the previous maximum indexed config in the store. Providing config as a prop solved this issue..

I'll actually put a comment in the code to this effect so we remember why we've done it this way.

EmmaLRussell · 2024-07-25T14:36:36Z

app/static/src/app/components/run/RunStochasticPlot.vue

@@ -29,6 +33,10 @@ export default defineComponent({
            type: Number,
            default: 0


It makes a sort of sense because RunPlot and RunStochasticPlot will always have fitPlot=false, so should have a valid index. But I think it would make even more sense to make this a required prop!

EmmaLRussell · 2024-07-25T14:40:13Z

app/static/src/app/components/sensitivity/SensitivitySummaryPlot.vue

@@ -54,7 +54,8 @@ export default defineComponent({

        const yAxisSettings = computed(() => {
            const isNotTimePlot = plotSettings.value.plotType !== SensitivityPlotType.TimeAtExtreme;
-            const logScale = store.state.graphs.settings.logScaleYAxis && isNotTimePlot;
+
+            const logScale = store.state.graphs.config[0].settings.logScaleYAxis && isNotTimePlot;


It does for now - we're going to implement multiple sensitivity plots in another ticket: https://mrc-ide.myjetbrains.com/youtrack/issue/mrc-5485/Include-all-Graphs-on-Sensitivity-tab

EmmaLRussell · 2024-07-25T14:42:07Z

app/static/src/app/components/sensitivity/SensitivityTracesPlot.vue

@@ -48,6 +51,7 @@ export default defineComponent({
        const store = useStore();

        const solutions = computed(() => store.state.sensitivity.result?.batch?.solutions || []);
+        const graphConfig = computed(() => store.state.graphs.config[0]);


Again, we'll sort this in mrc-5485, it can just look at the first config until then.

EmmaLRussell · 2024-07-25T14:42:50Z

app/static/src/app/store/graphs/state.ts

+    logScaleYAxis: false,
+    lockYAxis: false,
+    yAxisRange: [0, 0]
+});


Excellent point, will add some comments

EmmaLRussell · 2024-07-25T14:51:33Z

Role-playing as a QA tester. Do we want to set limits on what people can do their scales?

We've generally taken the approach of giving users the option to get themselves into trouble as a didactic opportunity!

EmmaLRussell · 2024-07-26T12:12:43Z

As discussed yesterday, I've added a ticket for the behaviour where y axis range selection should be overriden by new variable selection on the graph: https://mrc-ide.myjetbrains.com/youtrack/issue/mrc-5560

Also as discussed, I think it's ok for the graphs to revert to 0 as minimum y value as we don't expect any negative values.

M-Kusumgar

Ive left an optional comment, it would be work but i think it would be cleaner too

M-Kusumgar · 2024-07-26T15:11:34Z

app/static/src/app/components/WodinPlot.vue

+        // We could look up graphConfig from graphIndex - however this causes a specific reactivity bug
+        // when the last plot is deleted and the plot tries to observe the old maximum index before it is updated.
+        graphConfig: {
+            type: Object as PropType<GraphConfig | null>,
+            required: true
+        },


so i think using the index as a unique identifier is the problem here, i think the only piece of information all these components need to be passed down is the GraphConfig.id and use .finds or .findIndexs to locate where we are, this removes the need for graphIndex and graphConfig to be passed down and any component can just get that from the store, this will deal with the deleted graph hopefully as you can put that logic after the find. Also the store mutation will be alright too as you can just pass in the id instead of the index into the mutation

on this point i think the logic for updating graph settings and seeing whether we are at the fit plot or not can be extracted out into a composable that gets whether we are fit plot or not straight from the store based on what tab is selected, i think it would be cleaner

thats probably a lot of work so see if you feel up to these changes or not, happy for this to be ignored

Yeah, I definitely had issues with just using index, and tbh I thought needing to use findIndex felt dirtier than passing in graphIndex which we already know as well as the config - which just goes to show that cleanliness can be subjective!

I agree on the composable though, good to keep this dubious logic in one place either way - will make a ticket.

https://mrc-ide.myjetbrains.com/youtrack/issue/mrc-5572/make-a-composable-for-shared-plot-logic

EmmaLRussell and others added 5 commits June 27, 2024 10:40

lint!

ab89d93

prettier...

82dc1cc

Merge pull request #206 from mrc-ide/mrc-5440

3dbae1f

mrc-5440 Store changes preliminary to multiple graphs

per graph settings, with separate fit plot settings

4200367

styling

2b4b367

EmmaLRussell changed the title ~~mrc-5484 Per-graph settings, with separate fit plot settings~~ mrc-5442Per-graph settings, with separate fit plot settings Jul 16, 2024

EmmaLRussell changed the title ~~mrc-5442Per-graph settings, with separate fit plot settings~~ mrc-5442 Per-graph settings, with separate fit plot settings Jul 16, 2024

EmmaLRussell changed the base branch from mrc-5484 to mrc-5490 July 16, 2024 16:09

EmmaLRussell added 9 commits July 17, 2024 10:07

fix delete bug, provide graph config to plots as props

0e3f903

do not show graph configs on Options Tab if Fit tab is open

bf59118

unit tests

0ae34f5

fix e2e tests

e8f63d6

add e2e tests

6e3c103

lint etc

d8de6d8

Merge branch 'main' into mrc-5442

8ad1cf7

merge with epic branch

4327ff1

formatting

3aa1ae2

EmmaLRussell requested review from M-Kusumgar and david-mears-2 July 25, 2024 09:25

EmmaLRussell marked this pull request as ready for review July 25, 2024 09:34

david-mears-2 reviewed Jul 25, 2024

View reviewed changes

EmmaLRussell commented Jul 25, 2024

View reviewed changes

EmmaLRussell added 2 commits July 25, 2024 15:52

changes following review

79a700a

fix unit tests

80d7375

EmmaLRussell requested a review from david-mears-2 July 25, 2024 15:23

david-mears-2 approved these changes Jul 26, 2024

View reviewed changes

EmmaLRussell merged commit 4662e4f into mrc-5490 Jul 26, 2024
2 checks passed

M-Kusumgar reviewed Jul 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mrc-5442 Per-graph settings, with separate fit plot settings #210

mrc-5442 Per-graph settings, with separate fit plot settings #210

EmmaLRussell commented Jul 9, 2024 •

edited

Loading

david-mears-2 left a comment

david-mears-2 Jul 25, 2024 •

edited

Loading

EmmaLRussell Jul 25, 2024

david-mears-2 Jul 25, 2024

EmmaLRussell Jul 25, 2024

david-mears-2 Jul 25, 2024

david-mears-2 Jul 25, 2024

EmmaLRussell Jul 25, 2024 •

edited

Loading

david-mears-2 Jul 25, 2024

EmmaLRussell Jul 25, 2024

david-mears-2 Jul 25, 2024

david-mears-2 Jul 25, 2024

EmmaLRussell Jul 25, 2024

david-mears-2 Jul 25, 2024

EmmaLRussell Jul 25, 2024

david-mears-2 Jul 25, 2024

EmmaLRussell Jul 25, 2024

david-mears-2 commented Jul 25, 2024

EmmaLRussell left a comment

EmmaLRussell Jul 25, 2024

EmmaLRussell Jul 25, 2024

EmmaLRussell Jul 25, 2024 •

edited

Loading

EmmaLRussell Jul 25, 2024

EmmaLRussell Jul 25, 2024

EmmaLRussell Jul 25, 2024

EmmaLRussell Jul 25, 2024

EmmaLRussell commented Jul 25, 2024

EmmaLRussell commented Jul 26, 2024

M-Kusumgar left a comment

M-Kusumgar Jul 26, 2024

EmmaLRussell Jul 30, 2024 •

edited

Loading

EmmaLRussell Jul 30, 2024

		@@ -29,6 +33,10 @@ export default defineComponent({
		type: Number,
		default: 0

mrc-5442 Per-graph settings, with separate fit plot settings #210

mrc-5442 Per-graph settings, with separate fit plot settings #210

Conversation

EmmaLRussell commented Jul 9, 2024 • edited Loading

david-mears-2 left a comment

Choose a reason for hiding this comment

david-mears-2 Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EmmaLRussell Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-mears-2 commented Jul 25, 2024

EmmaLRussell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EmmaLRussell Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EmmaLRussell commented Jul 25, 2024

EmmaLRussell commented Jul 26, 2024

M-Kusumgar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EmmaLRussell Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EmmaLRussell commented Jul 9, 2024 •

edited

Loading

david-mears-2 Jul 25, 2024 •

edited

Loading

EmmaLRussell Jul 25, 2024 •

edited

Loading

EmmaLRussell Jul 25, 2024 •

edited

Loading

EmmaLRussell Jul 30, 2024 •

edited

Loading