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

[draft] Passing ax to some plot functions #50

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

balopat
Copy link

@balopat balopat commented Jan 9, 2025

Take this PR as a draft regarding ax in the plotting API.

A bug: ax is unused currently in plot_tree_rubberband - so I fixed that.

I also passed ax in

  • plot_tree_circuit
  • plot_tree_flat
  • plot_tree_contractions

The logic in all cases is that ax takes precedence over the passed in figsize if available and fig =None is returned.

I find this useful as it helps to create multiplot subplots like in #49.

Open questions:

  • would you like to have ax consistently across all plot functions?
  • would the figsize / fig logic described above be sufficient?
  • I don't like that rubberband shows the plot and there is no way to close it or force it to close (I'm working on WSL and the command line, not in a notebook env and it keeps popping up) - can we modify that too maybe by default turning show() off when ax is passed?
  • I guess these would need some testing too ideally, though plotting testing can get a bit hairy so maybe it's not worth it, let me know your thoughts on that too

@jcmgray
Copy link
Owner

jcmgray commented Jan 10, 2025

Thanks for the PR! I think yes generally desirable to be able to pass in an ax kwarg, happy to have that added/fixed on any plotting functions.

Yes I agree the logic should be that if an ax is supplied then fig kwargs are ignored and fig=None is returned. The show_and_close decorator should then not automatically pyplot.show(fig) the plot.

At the moment, fig is retrieved from Drawing.fig which actually retrieves it from ax.figure so show_and_close always sees a figure returned and displays it. One could add the ax logic to show_and_close or simply handle setting fig=None in the plotting functions that use Drawing.

Regarding testing, yes ideally just any basic test that shows the functions run without erroring (i.e. don't worry about explicit tests that plot output is as expected etc.). However, I wouldn't consider it required.

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