added functionality to obtain subplots from already created plots.

[ishanaj]

This PR gives a solution to the problem as discussed in Issue #15328
A class PlotGrid have been introduced in plot.py of plotting module. This class takes some already created Plot objects and makes a subplot figure usingmatplotlib backend.

TODO's:

• Improve documentation
• Add tests

Release Notes

• plotting
  • plotting module can now create subplots i.e can plot more than one plot in a single figure using PlotGrid

[sympy-bot]

✅

Hi, I am the SymPy bot (v145). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

• plotting
  • plotting module can now create subplots i.e can plot more than one plot in a single figure using PlotGrid (#16276 by @ishanaj)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.5.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

This PR gives a solution to the problem as discussed in Issue #15328
A class `PlotGrid` have been introduced in `plot.py` of plotting module. This class takes some already created `Plot` objects and makes a subplot figure using`matplotlib` backend.

TODO's:
- [x] Improve documentation
- [x] Add tests

#### Release Notes

<!-- BEGIN RELEASE NOTES -->
- plotting
  - plotting module can now create subplots i.e can plot more than one plot in a single figure using `PlotGrid`
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

[ishanaj]

The PR is not completely finished, but I would need some reviews regarding what has been implemented.
@Krastanov

[oscargus]

Can you please add some screen shots here to see the effect of it? Looks interesting!

[oscargus]

Also, some simple usage example in the documentation (or here) may help for trying it out.

[ishanaj]

I have tested the following example:

>>> from sympy import symbols
>>> x, y = symbols('x, y')
>>> p1 = plot(x, x**2, x**3, (x, -5, 5), show=False)
>>> p2 = plot((x**2, (x, -6, 6)), (x, (x, -5, 5)), show=False)
>>> p3 = plot(x**3, (x, -5, 5), show=False)
>>> p4 = plot3d(x*y, (x, -5, 5), (y, -5, 5), show=False)
>>> p = PlotGrid(2, 2, p1, p2, p3, p4)

This gives the following output:

[oscargus]

I realize that most things I commented on are from the old code...

Either way, I think it looks very promising! However, I do not really have much insight into this part of the code so there are probably more suitable people for commenting on that...

[oscargus]

Yes, this is an issue with 2.7... Could one possibly consider having the plots in a list? Not sure what is the better option.

[oscargus]

If you merge the current master you will not get this seemingly failed check as it is removed (from there).

[codecov]

Codecov Report

Merging #16276 into master will increase coverage by 0.483%.
The diff coverage is 9.574%.

@@             Coverage Diff              @@
##            master   #16276       +/-   ##
============================================
+ Coverage   73.257%   73.74%   +0.483%     
============================================
  Files          618      619        +1     
  Lines       158200   158698      +498     
  Branches     37175    37196       +21     
============================================
+ Hits        115893   117025     +1132     
+ Misses       36783    36255      -528     
+ Partials      5524     5418      -106

[Krastanov]

Hello, all. I wrote this plotting module a few years ago, but I have not been involved with SymPy development for a while, so take my comments with a grain of salt.

The changes seem generally correct to me. The gist of it is "add another level of indirection, and now loop over a list of ax objects instead of assuming only one ax object".

There is a large set of plotting tests that probably need to be inspected visually before accepting this for merging. Originally, there were also a couple of plotting example notebooks that should be inspected visually by a human as well.

I have only one negative comment, but if the core devs deem it nitpicky feel free to disregard it (it is somewhat stylistic too): A major part of the change involves mostly adding another level of indentation to already existing code. This is generally a reliable sign that a smaller, more self-contained change might be possible with the same effect. I believe here this is the case: the code will be more legible if instead of making the process_series function more complicated, you just make the process_series accept an ax object and have another function call this new (but simple) process_series multiple times.

Another sign that this might be a good idea is this piece of code. I believe it would be easier to follow if this is again part of the proposed function that calls process_series:

    if isinstance(self.parent, Plot):
        series_list = [self.parent._series]
    else:
        series_list = self.parent._series

However, there is a good argument against doing what I am suggesting: it bakes in the assumption that ax objects exist, tying you to a matplotlib backend. Originally the plotting module was designed to support many different backends, and some toy examples were made (for instance there was a toy D3.js backend). If backend independence is desired, one should be more careful and either disregard my suggestion from above or modify it appropriately.

Either way, thanks for updating this code!

[Krastanov]

Another thing that might need to be considered: is the API suggested here extendable to permit things like shared axes, or subplot grids of various sizes?

[Krastanov]

@ishanaj, thanks for following the suggested changes. In general feel free to push back against such suggestions: you are the one that has made the changes and the code is freshest in your mind, not mine. In this case I think the changes were good (look at the number of changed lines in the diff - now it is noticeably fewer changes).

I could continue nitpicking about the choice of function names (which one starts with _, which does not, whether the new function is descriptive enough, etc). There is plenty that can be "more optimal". But I doubt that discussion would be particularly productive, and it can be postponed to the day a second backend is implemented (but confer that is true with the actual core developers - we do not want to burden them with an annoying API set in stone for them to support).

Anyway, if a human has checked visually what the test results for the old plots are, I am in favor of this being merged.

[ishanaj]

Another thing that might need to be considered: is the API suggested here extendable to permit things like shared axes, or subplot grids of various sizes?

For shared axes, I think, one thing that could be done is taking sharex as a keyword argument in Plot class, which could later be checked in MatplotlibBackend and accordingly be defined as an argument in add_subplot(). Something like:

>>> p1 = plot(sin(2*pi*x), show=False)
>>> p2 = plot(sin(4*pi*x), sharex=p1, show=False)

And yes, it does permit to have subplot grids of various sizes. Although it currently cannot append or extend the size of the grid. I have added some examples in documentation

[ishanaj]

Anyway, if a human has checked visually what the test results for the old plots are...

@Krastanov Do I also have to add some tests in plot_test.py?
Because I am a bit confused in dealing with the plot_test.py file regarding how it performs the test.
It would be great if I get some hints about its way of working.

[ishanaj]

ping @smichr @asmeurer

[Krastanov]

@ishanaj I think the plot_test.py has been modified since the last time I have used it... You might be able to see the plots it creates saved in a temporary folder somewhere (/tmp on a Linux machine for instance), but I do not know how convenient that is. It seems it is mostly there to ensure that a plot is created, without really checking that the plot looks reasonable. This is a good enough first test and I would encourage you put at least a couple examples of your own code being used there, to ensure there are not show-stopper bugs introduced in the future.

I would suggest opening the notebooks related to plotting from here: https://github.com/sympy/sympy/tree/d383496deb5bed647ed930fd0194c0f57dba495d/examples/beginner and checking whether they look reasonable with all the changes you have made. I would be surprised if there are any problems, but better safe than sorry.

[ishanaj]

@Krastanov I have checked most of the examples and tallied the results with the old plot. There seems to be no difference in the results. I guess we are good to go.
ping @smichr @oscargus @sylee957

[Krastanov]

Everything sounds great to me. From my, admittedly outdated, experience with the project, this seems like a neat pull request worthy of merging.

[ishanaj]

I have corrected a minor bug and also have added fig.tight_layout() for better spacing between subplots

[moorepants]

Great work here. I left a few comments. I like the API.

[ishanaj]

Also, I found out that the current master(and this branch) are not giving results for the sqrt and LambertWfunctions mentioned here.
For plot(LambertW(x)) they give simply give the axes without any curve.

Although SymPy version 1.3 does give a result for the same.

EDIT: I opened an issue regarding the same #16572 .

[ishanaj]

@moorepants I have made the required changes. Please have a look.

[moorepants]

I'm good with this PR. Nice work!

[ishanaj]

Sorry, I forgot to include the documentation in the .rst file.

[ishanaj]

I think the PR is good to go now!

[moorepants]

@oscargus and @Krastanov Are you all happy with this? If so I'll merge (or you can).

[Krastanov]

Yes, my last comment stands. This seems well done to me.

[moorepants]

I'm going to merge. Thanks everyone.

[ishanaj]

Thanks!