Skip to content
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-5484 Link x-axes across multiple graphs #209

Merged
merged 21 commits into from
Jul 16, 2024
Merged

mrc-5484 Link x-axes across multiple graphs #209

merged 21 commits into from
Jul 16, 2024

Conversation

EmmaLRussell
Copy link
Contributor

@EmmaLRussell EmmaLRussell commented Jul 4, 2024

This branch implements synchronising the x axes when there are multiple graphs on the Run Tab - in other words, when the user zooms in on one graph, changing the x axis zoom from the default, the other graphs should likewise zoom to the same x axis range, so the user can view values for all variables across the same time sub-range.

The mechanism for doing this is through a shared linkedXAxis prop which is emitted to the parent tab, and propagated to all child graphs by updating the prop value. This prop is optional, as not all graphs have linked x axes (it's not yet in Sensitivity, and possibly won't ever be in Fit).

I've lightly refactored the plot updating code, which was already a bit of a confusing tangle, and added some comments which I find meaningful, but let me know if that doesn't work for you! I have added a lastYAxisFromZoom value to the plot, which enables a previous Y-only zoom to be retained, across an x-axis emit and propagate. This value will be overriden by locked y axis range, if any.

Lock y axis currently behaves weirdly if you have multiple graphs because the final graph's current y axis range is used and propagates to the other graphs. This will be fixed in the next ticket when we have separate graph settings for each graph.

We also set the right margin on the plot so that legend width is fixed for all graphs. We currently just use a heuristic to calculate a reasonable width from the maximum variable length.

As mentioned in standup, I've added an id to the GraphConfig which can be used as a key for the plot component - this is so that the component and its internal state (particularly lastYAxisFromZoom) can be retained when an earlier plot is deleted (which is not the case if you just use the array index as the key). This id is a generated uid, which is not serialised (so that sessions which only differ by this id are correctly identified as duplicates).

Some outstanding issues:

  • Should we reset 'lastYAxisFromZoom' if add new variable to graph? i.e. should user's y-axis zoom selection be overridden when a new variable appears whose values may not intersect with the current y zoom?
  • Unrelated to x axis, but something I've just noticed for Fit apps - after link, should we only render data points on graphs with data variable selected? (And only show sum of squares on that graph too?) Update: yes, we'll make a ticket to do this.
  • The 'reset axes` button currently resets the x axis but not the y axis. I think this is because we've deliberately remembered and specified the y range, and so we get that y range in the relayout event from the 'reset axes' button - whereas we actually want to go back to autorange for both axes. I think we can fix that by putting in a custom handler for that button - but I'd prefer to do that in another ticket.
  • Legend width is going to need tweaking for stochasitc, as that adds (mean) to some variable labels... It's also going to be wider than it needs to be for Fit potentially, which does not show all selected variables, only the linked ones.

@EmmaLRussell EmmaLRussell marked this pull request as ready for review July 9, 2024 08:40
Copy link
Collaborator

@absternator absternator left a comment

Choose a reason for hiding this comment

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

LGTM good work with this finicky stuff... works great too

}
},
components: {
WodinPlot
},
setup(props) {
emits: ["updateXAxis"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

best to type as well 😄

};
};

const relayout = async (event: PlotRelayoutEvent) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const relayout = async (event: PlotRelayoutEvent) => {
const reLayout = async (event: PlotRelayoutEvent) => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'm going to leave this as is, since this is a Plotly word, and it doesn't capitalise the l wherever 'relayout` appears, so I think it would be inconsistent if we did.

};

const relayout = async (event: PlotRelayoutEvent) => {
if (event["xaxis.autorange"] || (event["xaxis.range[0]"] && event["xaxis.range[1]"])) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

whats the reason for accessing with [] brackets? and not with dot notation? which is much safer 😄

Copy link
Contributor Author

@EmmaLRussell EmmaLRussell Jul 16, 2024

Choose a reason for hiding this comment

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

Because for some reason, this is a flat single level dictionary object but the dictionary keys are things like "xaxis.range[0]", implying a more nested structure, so it wouldn't work if you just went event.xaxis.range[0] as it would expect to find that non-existent nested structure... I don't really know why Plotly have done it this way.. but they have!

const longestVar = graphsGetters[GraphsGetter.allSelectedVariables].sort((a: string, b: string) =>
a.length < b.length ? 1 : -1
)[0];
return longestVar.length * 10 + 40;
Copy link
Collaborator

Choose a reason for hiding this comment

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

would be get these names as variables

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean unmagic these numbers? Will do.

@EmmaLRussell EmmaLRussell merged commit 3e13370 into mrc-5490 Jul 16, 2024
2 checks passed
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.

2 participants