Skip to content

Add an option to pass an existing figure to get_grid #55

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

PeterMinin
Copy link

In case you decide that passing a figure in is ok (#54), here is an implementation :)
Thank you for this project!

@pganssle
Copy link
Member

So I think that gridspec.GridSpec doesn't actually require a figure, so I'm thinking maybe it's better to get out of the business of using figures at all if we can help it.

I have to take a closer look at the code to see what that will mean for the mechanism we're using to produce the GridSpec, though, and for how convenient the module is to use.

Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me, but leave to @pganssle if you want to remove the usage of figure all together.

@pganssle
Copy link
Member

pganssle commented Mar 16, 2019

I think in the long term it might be best to re-factor all the stuff that requires a figure out into some separate mechanism, to make it easier to re-use this with other plotting systems, but since we're so tightly tied to matplotlib at the moment, we can go ahead with this. No need to make the perfect the enemy of the good.

@PeterMinin Any chance you can add a test for this? I think the best way is to mock out gridspec.GridSpec and test that it is passed the correct figure object.

Edit: Some examples of similar mocking are in tests/test_grids.py, which is where the test for this new behavior should go as well.

@PeterMinin
Copy link
Author

Yes, hopefully I'll be able to do it in a couple of days.

@pganssle pganssle self-requested a review March 20, 2019 20:20
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