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

Add support for multiplot options #52

Closed
wants to merge 1 commit into from

Conversation

tomwhatmore
Copy link
Contributor

I re-implemented the multiplot options from pull request #47 using @SiegeLord's feedback on the API and where to generate the gnuplot commands. I also added an example showing how to use the new options.

@SiegeLord
Copy link
Owner

Thanks! This looks better. I'll need to test things out more, but I'll start with this baseline. In particular, it's odd to me that some things are tied to the multi-layout which don't have to be (like title). I want to revisit whether we can always call set multiplot... it seems I disabled this in 2416ef8 for animation support, but I'm not completely sure why I did that.

@tomwhatmore
Copy link
Contributor Author

I also felt like there was some weird over-coupling with the multi-layout logic, but I deliberately avoided getting into the larger questions of where and when the project calls set multiplot because it seemed out of scope for this feature, and additionally I didn't feel like I fully understood the existing logic. Just adding the extra options to the existing call seemed much safer!

Not sure how much I can help with any larger refactoring, but if I can make any changes to this PR that would make it easier let me know.

@SiegeLord
Copy link
Owner

A small update on this. It appears to not be a problem to just use set multiplot all the time. A bigger problem is the code right now calls reset before every axis, which nukes the multiplot options and breaks the multiplot_options example. Removing reset makes things like X-labels etc bleed across plots. Fixing #40 will remove the need to call reset, but that's a somewhat big effort.

@SiegeLord
Copy link
Owner

I've merged this with a number of supporting changes, thanks for the initial work!

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