Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

[Work in Progress] Basic framework for restructured plot. #1468

Merged
merged 29 commits into from

5 participants

@catchmrbharath

This is a just a framework of functions that will replace plot which took care of all the possible plots.
The behaviour that is expected is provided in the docstrings. This pull request was made to get an opinion
on the names of the functions and the behaviour that has to be expected of these functions.

I will keep updating this branch with the code for the behaviour.

@travisbot

This pull request fails (merged 85cd5a9 into 65b6582).

sympy/plotting/plot_new.py
((5 lines not shown))
+ The plotting uses an adaptive algorithm which samples recursively to
+ accurately plot the plot. The adaptive algorithm uses a random point near
+ the midpoint of two points that has to be further sampled. Hence the same
+ plots can appear slightly different.
+
+ Usage
+ =====
+ plot_line((expr1, range), (expr2, range), ...)
+ plot_line(expr, range, ...)
+
+ Arguments
+ =========
+ ``expr`` : Expression representing the function of single variable
+ ``range``: (x, 0 , 5), A 3 - tuple denoting the range of the free variable.
+
+ If the ranges is not specified, then a default range of (-10, 10) is used.
@asmeurer Owner
asmeurer added a note

Maybe add: "This may change in the future if a more advanced default range detection algorithm is detected."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
sympy/plotting/plot_new.py
((71 lines not shown))
+
+ The plotting uses an adaptive algorithm which samples recursively to
+ accurately plot the plot. The adaptive algorithm uses a random point near
+ the midpoint of two points that has to be further sampled. Hence the same
+ plots can appear slightly different.
+
+ Usage
+ =====
+ plot_parametric((expr_x, expr_y, range), ...)
+ plot_parametric(expr_x, expr_y, range)
+
+ Arguments
+ =========
+ ``expr_x`` : Expression representing the function along x.
+ ``expr_y`` : Expression representing the function along y.
+ ``range``: (u, 0 , 5), A 3 - tuple denoting the range of the parameter
@asmeurer Owner
asmeurer added a note

No space before ,, no space around -.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
sympy/plotting/plot_new.py
((48 lines not shown))
+ ``xlim`` : tuple of two floats, denoting the x - axis limits.
+
+ ``ylim`` : tuple of two floats, denoting the y - axis limits.
+
+ Examples
+ ========
+ >>> plot_line(x**2, (x, -5, 5))
+
+ Multiple plots
+ >>> plot_line((x**2, (x, -6, 6)), (x, (x, -5, 5)))
+
+ Multiple plots with same range.
+ >>> plot_line(x**2, x, (x -5, 5))
+
+ No adaptive sampling.
+ >>> plot_line(x**2, adaptive = False, nb_of_points = 400)
@asmeurer Owner
asmeurer added a note

Don't put spaces around = when it's used to denote keyword arguments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@asmeurer
Owner

Why create a new file for this?

@Krastanov
Collaborator

SymPy Bot Summary: :red_circle: There were test failures.

@catchmrbharath: Please fix the test failures.

Test command: setup.py test
master hash: 98cc80f
branch hash: 85cd5a99c5f32d08e756b0c827c3c5c46086ea65

Interpreter 1: :red_circle: There were test failures.

Interpreter: /usr/local/bin/python2.5 (2.5.6-final-0)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYhcYiDA

Interpreter 2: :red_circle: There were test failures.

Interpreter: /usr/bin/python2.7 (2.7.3-candidate-2)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYlPAhDA

Interpreter 3: :red_circle: There were test failures.

Interpreter: /usr/bin/python3.2 (3.2.3-candidate-2)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYvbYiDA

Build HTML Docs: :eight_spoked_asterisk: All tests have passed.

Docs build command: make html-errors
Sphinx version: 1.1.3

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYiJQjDA

Automatic review by SymPy Bot.

@travisbot

This pull request fails (merged d9efa1ea into 65b6582).

@Krastanov
Collaborator

SymPy Bot Summary: :red_circle: There were test failures.

@catchmrbharath: Please fix the test failures.

Test command: setup.py test
master hash: 0d88b25
branch hash: d9efa1ea0a5efa812fe4e76e308cf3f6068b69c0

Interpreter 1: :red_circle: There were test failures.

Interpreter: /usr/local/bin/python2.5 (2.5.6-final-0)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYn-0iDA

Interpreter 2: :red_circle: There were test failures.

Interpreter: /usr/bin/python2.7 (2.7.3-candidate-2)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYkJQjDA

Interpreter 3: :red_circle: There were test failures.

Interpreter: /usr/bin/python3.2 (3.2.3-candidate-2)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYqowjDA

Build HTML Docs: :eight_spoked_asterisk: All tests have passed.

Docs build command: make html-errors
Sphinx version: 1.1.3

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYj5QjDA

Automatic review by SymPy Bot.

@asmeurer
Owner

SymPy Bot Summary: :red_circle: There were test failures (merged catchmrbharath/restructured_plot (d9efa1ea0a5efa812fe4e76e308cf3f6068b69c0) into master (0d88b25)).

@catchmrbharath: Please fix the test failures.

Interpreter 1: :red_circle: There were test failures.
Interpreter: None (2.5.0-final-0)
Architecture: Darwin (32-bit)

Interpreter 2: :red_circle: There were test failures.
Interpreter: None (2.6.6-final-0)
Architecture: Darwin (32-bit)

Interpreter 3: :red_circle: There were test failures.
Interpreter: None (2.7.2-final-0)
Architecture: Darwin (32-bit)

Interpreter 4: :red_circle: There were test failures.
Interpreter: /sw/bin//python2.6 (2.6.8-final-0)
Architecture: Darwin (64-bit)

Interpreter 5: :red_circle: There were test failures.
Interpreter: /sw/bin//python2.7 (2.7.3-final-0)
Architecture: Darwin (64-bit)

Interpreter 6: :red_circle: There were test failures.
Interpreter: None (3.2.2-final-0)
Architecture: Darwin (32-bit)

Interpreter 7: :red_circle: There were test failures.
Interpreter: None (3.3.0-beta-1)
Architecture: Darwin (32-bit)

Interpreter 8: :red_circle: There were test failures.
Interpreter: /sw/bin//python3.2 (3.2.3-final-0)
Architecture: Darwin (64-bit)

Interpreter 9: :red_circle: There were test failures.
Interpreter: None (3.3.0-beta-1)
Architecture: Darwin (64-bit)

Build HTML Docs: :red_circle: There were test failures.
Sphinx version: 1.1.3

@travisbot

This pull request fails (merged c3964eb2 into 65b6582).

@Krastanov
Collaborator

SymPy Bot Summary: :red_circle: There were test failures.

@catchmrbharath: Please fix the test failures.

Test command: setup.py test
master hash: b0a9866
branch hash: c3964eb2aca5d5a9a60dd2d5869e92135cb64ba2

Interpreter 1: :red_circle: There were test failures.

Interpreter: /usr/local/bin/python2.5 (2.5.6-final-0)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY8KMjDA

Interpreter 2: :red_circle: There were test failures.

Interpreter: /usr/bin/python2.7 (2.7.3-candidate-2)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYoMYiDA

Interpreter 3: :red_circle: There were test failures.

Interpreter: /usr/bin/python3.2 (3.2.3-candidate-2)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYsfAhDA

Build HTML Docs: :eight_spoked_asterisk: All tests have passed.

Docs build command: make html-errors
Sphinx version: 1.1.3

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYn8YiDA

Automatic review by SymPy Bot.

@travisbot

This pull request fails (merged 749e5664 into 65b6582).

@catchmrbharath

@asmeurer @Krastanov Can you have a look at this? Is the API access better?

I will add the depth and the nb_of_points in an hour.

sympy/plotting/plot.py
((12 lines not shown))
+ Plots a function of a single variable.
+
+ The plotting uses an adaptive algorithm which samples recursively to
+ accurately plot the plot. The adaptive algorithm uses a random point near
+ the midpoint of two points that has to be further sampled. Hence the same
+ plots can appear slightly different.
+
+ Usage
+ =====
+ Single Plot
+ plot_line(expr, range, ...)
+
+ Multiple plots with same range.
+ plot_line(expr1, expr2, ..., range)
+
+ Multiple plots with different .
@asmeurer Owner

ranges

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
sympy/plotting/plot.py
((170 lines not shown))
+ show = kwargs.pop('show', True)
+ series = []
+ plot_expr = check_arguments(args, 2, 1)
+ series = [Parametric2DLineSeries(*arg) for arg in plot_expr]
+ plots = Plot(*series, **kwargs)
+ if show:
+ plots.show()
+
+def plot3D_parametric(*args, **kwargs):
+ """
+ Plots a 3D parametric plot.
+
+ Usage
+ =====
+ Single plot
+ plot3D_parametric(expr_x, expr_y, expr_z, range, kwargs)
@asmeurer Owner

**kwargs

@asmeurer Owner

Are multiple plots with the same range supported here too?

No. I am not able to differentiate between tuple of expressions and tuple of ranges as both are of length 3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@asmeurer asmeurer commented on the diff
sympy/plotting/plot.py
((287 lines not shown))
+ ``line_color``: List of floats corresponding to the line color of the
+ expressions. The length of the List should match the number of expressions.
+
+ ``xscale``: {'linear', 'log'} Sets the scaling of the x - axis.
+
+ ``yscale``: {'linear', 'log'} Sets the scaling if the y - axis.
+
+ ``axis_center``: tuple of two floats denoting the coordinates of the center or
+ {'center', 'auto'}
+
+ ``xlim`` : tuple of two floats, denoting the x - axis limits.
+
+ ``ylim`` : tuple of two floats, denoting the y - axis limits.
+
+ Examples
+ ========
@asmeurer Owner

Put an empty line under all your header lines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@asmeurer
Owner

By the way, we need a Sphinx file for the new plotting module.

sympy/plotting/plot.py
((7 lines not shown))
+# TODO: Add support for depth and nb_of_points
+# TODO: Add color arrays for plots.
+
+def plot_line(*args, **kwargs):
+ """
+ Plots a function of a single variable.
+
+ The plotting uses an adaptive algorithm which samples recursively to
+ accurately plot the plot. The adaptive algorithm uses a random point near
+ the midpoint of two points that has to be further sampled. Hence the same
+ plots can appear slightly different.
+
+ Usage
+ =====
+ Single Plot
+ plot_line(expr, range, ...)
@asmeurer Owner

I think ... should be replaced with **kwargs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@asmeurer
Owner

Where the range is optional, the spec should read [, range].

@travisbot

This pull request fails (merged dea33703 into 65b6582).

@catchmrbharath

Where the range is optional, the spec should read [, range]

I didn't get it. Can you show with an example? Thanks.

@Krastanov
Collaborator

SymPy Bot Summary: :red_circle: There were test failures.

@catchmrbharath: Please fix the test failures.

Test command: setup.py test
master hash: b0a9866
branch hash: dea3370300d87c475356e6182911c876f77581b2

Interpreter 1: :red_circle: There were test failures.

Interpreter: /usr/local/bin/python2.5 (2.5.6-final-0)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY248iDA

Interpreter 2: :red_circle: There were test failures.

Interpreter: /usr/bin/python2.7 (2.7.3-candidate-2)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYjIAiDA

Interpreter 3: :red_circle: There were test failures.

Interpreter: /usr/bin/python3.2 (3.2.3-candidate-2)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY3bYiDA

Build HTML Docs: :eight_spoked_asterisk: All tests have passed.

Docs build command: make html-errors
Sphinx version: 1.1.3

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY-9UiDA

Automatic review by SymPy Bot.

@travisbot

This pull request fails (merged d38f44ff into b0a9866).

@travisbot

This pull request fails (merged d38f44ff into b0a9866).

@Krastanov
Collaborator

SymPy Bot Summary: :red_circle: There were test failures.

@catchmrbharath: Please fix the test failures.

Test command: setup.py test
master hash: b0a9866
branch hash: d38f44ff5b160ab13b66aa52d0b11372b15804db

Interpreter 1: :red_circle: There were test failures.

Interpreter: /usr/local/bin/python2.5 (2.5.6-final-0)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYtPAhDA

Interpreter 2: :red_circle: There were test failures.

Interpreter: /usr/bin/python2.7 (2.7.3-candidate-2)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYkfUiDA

Interpreter 3: :red_circle: There were test failures.

Interpreter: /usr/bin/python3.2 (3.2.3-candidate-2)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYwowjDA

Build HTML Docs: :eight_spoked_asterisk: All tests have passed.

Docs build command: make html-errors
Sphinx version: 1.1.3

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY_tUiDA

Automatic review by SymPy Bot.

@travisbot

This pull request fails (merged a764547d into 9215c28).

@Krastanov
Collaborator

SymPy Bot Summary: :red_circle: There were test failures.

@catchmrbharath: Please fix the test failures.

Test command: setup.py test
master hash: 9215c28
branch hash: a764547dbd89b69d056ace82ffa204dc8ea2540a

Interpreter 1: :red_circle: There were test failures.

Interpreter: /usr/local/bin/python2.5 (2.5.6-final-0)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY2qsjDA

Interpreter 2: :red_circle: There were test failures.

Interpreter: /usr/bin/python2.7 (2.7.3-candidate-2)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY9aMjDA

Interpreter 3: :red_circle: There were test failures.

Interpreter: /usr/bin/python3.2 (3.2.3-candidate-2)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY4LYiDA

Build HTML Docs: :eight_spoked_asterisk: All tests have passed.

Docs build command: make html-errors
Sphinx version: 1.1.3

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYsLMjDA

Automatic review by SymPy Bot.

@catchmrbharath

The new API is complete. I have added the tests and also updated the notebooks with the new functions

@asmeurer @Krastanov One of things I was not sure of was whether to delete the present plot function. There are very good cases of plot, for example, while plotting the solutions of equations, where we don't know whether the plot is going to be 3D or 2D. Any suggestions / discussion on whether it should be removed or not?

@asmeurer
Owner

I vote for removing it.

By the way, this can't be merged cleanly.

@catchmrbharath

I will rebase it.

catchmrbharath added some commits
@catchmrbharath catchmrbharath Basic framework of restructured plot 4b0755f
@catchmrbharath catchmrbharath Moved plot_new to plot.py 8b9033f
@catchmrbharath catchmrbharath Basic code for the new plotting API 9ee3251
@catchmrbharath catchmrbharath New plot functions added
* ``plot_line`` plots 2D lines
* ``plot_parametric`` plots 2D parametric lines
* ``plot3D`` plots 3D plots of expression of 2 variables
* ``plot3D_parametric`` plots 3D parametric line plots
* ``plot3D_surface`` plots 3D surfaces defined by 2 parameters
3727dc3
@catchmrbharath catchmrbharath Documentation changes
* Addressed the comments
* Added information about which keyword arguments belong to which
class
fde8795
@catchmrbharath catchmrbharath Documentation changes.
* Addressed the comments.
* Add more information about default range.
0a92087
@catchmrbharath catchmrbharath Added support for depth and nb_of_points
* depth - recursion depth for adaptive sampling
* adaptive - set to false, if uniform sampling is needed
* nb_of_points - number of points the range has to be uniformly sampled into.

Changes to the documentation
Removed the parameters that are not defined for 3D plots.
c21fa31
@catchmrbharath catchmrbharath added tests for new plotting functions
* added a global variable _show in plot module, if set to False
doesn't plot the graph, by default. This can be used in tests
where we have just save the plots without showing.

* changed tests so that they use the new plotting functions.
9acb6c1
@catchmrbharath catchmrbharath Changed the notebook to use the new notebook.
* Removed all instances of plot
* Added examples for each plot_* function
3b74c0e
@catchmrbharath catchmrbharath Replace all instances of plot with plot_line 6db3d67
@catchmrbharath catchmrbharath replaced instances of plot with plot3D_surface 46e0f52
@catchmrbharath catchmrbharath Replace all instances of plot with their corresponding equivalents 11ad486
@catchmrbharath catchmrbharath replace ``plot`` with the new functions
* Also removed the ``nsolve`` example which was failing.
f1aad78
@catchmrbharath catchmrbharath implicit plot tests with show unset
Also fixed a problem with xlim
38c4752
@travisbot

This pull request fails (merged 38c4752 into b52b7c3).

@Krastanov
Collaborator

SymPy Bot Summary: :red_circle: There were test failures.

@catchmrbharath: Please fix the test failures.

Test command: setup.py test
master hash: b52b7c3
branch hash: 38c4752

Interpreter 1: :red_circle: There were test failures.

Interpreter: /usr/local/bin/python2.5 (2.5.6-final-0)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYkpwjDA

Interpreter 2: :red_circle: There were test failures.

Interpreter: /usr/bin/python2.7 (2.7.3-candidate-2)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYqMYiDA

Interpreter 3: :red_circle: There were test failures.

Interpreter: /usr/bin/python3.2 (3.2.3-candidate-2)
Architecture: Linux (64-bit)
Cache: yes

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY38IjDA

Build HTML Docs: :eight_spoked_asterisk: All tests have passed.

Docs build command: make html-errors
Sphinx version: 1.1.3

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYs7MjDA

Automatic review by SymPy Bot.

@asmeurer
Owner

It looks like you just need some imports in your doctests.

examples/beginner/plot_advanced.ipynb
@@ -2,25 +2,37 @@
"metadata": {
"name": "plot_advanced"
},
- "nbformat": 2,
+ "nbformat": 3,
@asmeurer Owner

The notebooks should stay in version 2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@asmeurer
Owner

I think there will be some issues with your docstrings in Sphinx, but we can figure that out when you create a Sphinx doc for the new plotting module.

@Krastanov
Collaborator

I have written a comment 4 days ago, however it seems that with the new reply addresses in github it had never been posted (I have written it as a reply to an email)

Here it is (some of it is outdated):

old comment

There is a number of things that I dislike about how this evolves (all
the comments below are my opinion, not a final verdict - the rest of
the community may very well disagree):

See number 4 as it may be the most important.

  1. I continue to find the names for the 3D stuff confusing (check my
    previous comment for suggestions)

    • plot3D plots a non-parametric surface - ok
    • plot3D_surface plots a parametric surface - confusing
    • plot3D_parametric plots a 3D line - confusing
  2. While very nicely formated, the docstrings are full of
    repetitions. Worse yet, they repeat something that is said already in
    other classes. All the repetitions should be moved to the docstring of
    a single class or to the module docstring, otherwise it would be a
    nightmare to keep them up to date with the code. All the options
    should just be dumped in **kwargs and the docstring should say that
    the **kwargs are simply transfered to the Plot class or whatever.
    The user can then check its docstring

  3. This does not make any distinction between, nor does it mention
    per-plot options (eg title, axis label), per-series options (eg line
    label, adaptive, depth, nb_of_points), and per-series aesthetics (eg
    coloring functions). However, this is a very important notion behind
    the architecture of the module. I think that the best way forward
    would be for the per-plot options to be left as kwargs (as it is at
    the moment), and the rest to be left out, because there is already
    another way to do it (this means removing the TODO about nb_of_points
    and depth, and removing the adaptive kwarg).

  4. This reimplements many parts of the plot function (eg the
    check_arguments function). In addition it has a number of regressions
    with respect to it (the plot function gives greater freedom in the
    form of the arguments, including the formats that Aaron requested).
    Basically, I do not see what this api does better besides
    documentation. All the functions have the same calling signatures as
    the appropriate plot call and do less things. Is the only advantage
    the fact that we have more functions with smaller scope? If so,
    plot() should be removed and its internals should be abstracted away
    and used inside these functions in order to regain the generality that
    plot provides

  5. This repeats what is done in the Series factory class. It may or
    may not be done in a better way, however whichever one is chosen, the
    other must be removed or the common parts must be abstracted away.

  6. The check_arguments function seems excessively long and
    complicated for what it does. Compare it again with what is done
    inside the plot function, which while more general is much shorter
    (thanks to the additions that Chris made).

Most of the issues are concerned with code/documentation repetitions
and I consider such issues to be serious blockers for the moment. I
don't like the idea of merging this before removing/refactoring the
already implemented equivalent API based around plot() and the
Series factory class. And I definitely do not like the idea of
completely rewriting it - after all the testing and extensions done to
plot by me and Chris it would be a waste to rewrite it in a way that
does not support all the options that have been already implemented.

I suppose that a good starting point would be to move the plot
function and the factory class to this new file and start abstracting
it away line by line.

end of old comment

@Krastanov
Collaborator

About removing plot - I vote for removing it and the Series factory class. However I need to check whether your newest additions address my comments.

@Krastanov
Collaborator

I have given it another look:

  1. The issue of documentation and the issue of not accounting for the difference between per-plot options, per-series option and per-series aesthetics is still there. This will be a great burden to support in the future, especially if somebody else tries to contribute. As any future contributor will not be aware of the conventions, the code will quickly grow unmaintainable (exactly as it has happened with lambdify for instance). In the same line of thought, it may be a good idea to comment clearly in the code where the options are defined.

  2. This still does only a subset of all that plot does while it is longer in lines of code. I am ok to see plot removed, as obviously most people prefer smaller functions with more specific scope, however there is quite a bit of code repetition at the moment that was never necessary for plot. I would like to see the useful parts of plot abstracted and reused in this regard.

Anyway, I admit that I may be wrong about point 2, however I would like to be proven wrong. On the other hand I feel very strongly about the documentation issue in point 1.

P.S. When you remove plot you should also remove the Series factory class.
P.P.S. I actually preferred the use of a separate file, as 2000 lines of code is a bit too much. Anyway, it is to late to complain :)
P.P.P.S. I slightly dislike the use of the global _show variable. It does indeed seems useful, however now there is both kwarg['show'] and global _show. I always disliked this in matplotlib (the ion and ioff functions). This is something for the interpreter to magically address (like ipython) not for the library to create special cases.

@asmeurer
Owner

All the options should just be dumped in *kwargs and the docstring should say that the *kwargs are simply transfered to the Plot class or whatever. The user can then check its docstring

A potential usability issue with this is that Plot will still point to the old Plot by default, which would most likely confuse most users.

@asmeurer
Owner

The global show variable is so we can write doctests without having to use show=False or skip.

@catchmrbharath

I continue to find the names for the 3D stuff confusing (check my
previous comment for suggestions)

Yeah they are confusing. Can you send your suggestions for the names.
I couldn't find a better name.

The issue of documentation and the issue of not accounting for the difference between per-plot options, per-series option and per-series aesthetics is still there. This will be a great burden to support in the future, especially if somebody else tries to contribute. As any future contributor will not be aware of the conventions, the code will quickly grow unmaintainable (exactly as it has happened with lambdify for instance). In the same line of thought, it may be a good idea to comment clearly in the code where the options are defined.

I have mentioned it in the docstrings of the new plot functions, that certain arguments are for the respective Series class and Plot class. The kwargs is passed to both these classes. I think this is what you mentioned in the comments. Please correct me if I haven't addressed this.

@catchmrbharath

I actually preferred the use of a separate file, as 2000 lines of code is a bit too much. Anyway, it is to late to complain

Actually I was thinking of moving the matplotlib backend to a backends.py file.

@catchmrbharath

This still does only a subset of all that plot does while it is longer in lines of code. I am ok to see plot removed, as obviously most people prefer smaller functions with more specific scope, however there is quite a bit of code repetition at the moment that was never necessary for plot. I would like to see the useful parts of plot abstracted and reused in this regard.

The check_arguments uses a lot of plot 's code, but I will still try refractoring it.

@catchmrbharath

@asmeurer @Krastanov Does it make sense to allow only only one plot to be plotted per function, now that we have interactive plotting? We can always extend new plots using p.extend(new_plot).

@asmeurer
Owner
@Krastanov
Collaborator

Concerning whether to use Plot.expand and Plot.append for plotting multiple graphs interactively - I dislike the idea. While these two functions are in the backbone of the module and are the only sane choice for library code, for interactive use they are indeed cumbersome.

While on this topic: in this PR the only way to plot, for instance, "parametric 3d line over a 3d surface" or "parametric 2d line and a simple line plot" is to use "Plot.append" and "Plot.extend". In the plot function from master, this was possible by simply supplying the necessary arguments. I see arguments for and against both approaches so I do not have a strong opinion.

Concerning:

Yeah they are confusing. Can you send your suggestions for the names. I couldn't find a better name.

Sorry, I have failed to correctly send a number of comments after the change in the notification system. I have suggested plot_3d_parametric_line, plot_surface, plot_parametric_surface, however these are probably not the best names either.

Concerning:

Actually I was thinking of moving the matplotlib backend to a backends.py file.

It sounds good to me. There is also the text backend.

Concerning the comment by Aaron:

A potential usability issue with this is that Plot will still point to the old Plot by default, which would most likely confuse most users.

Then all the additional docs can be in the module docstring, not in Plot's docstring

Finally about:

I have mentioned it in the docstrings of the new plot functions, that certain arguments are for the respective Series class and Plot class. The kwargs is passed to both these classes. I think this is what you mentioned in the comments. Please correct me if I haven't addressed this.

I am still very worried that the different types of options are not clearly explained. I would like to see some part of the docs explaining the difference between:

  • per-plot options (title, xlabel, ylabel)
  • per-series options (label, adaptive, nb_of_points, depth)
  • per-series aesthetics (line_color, surface_color)

This is a fundamental part of the plotting module and the only reason for the complex class structure. It permits a great amount of flexibility that should be documented for the users and for any potential contributors. I think that I have done an average job while explaining it in the docstrings and comments inside Plot and before or inside of BaseSeries. As your new api touches upon these properties it should be explained how do they work.

@Krastanov
Collaborator

Concerning whether to use Plot.expand and Plot.append for plotting multiple graphs interactively - I dislike the idea. While these two functions are in the backbone of the module and are the only sane choice for library code, for interactive use they are indeed cumbersome.

To be clear, I dislike the idea if this is supposed to be the only way. While it would be nice

@Krastanov Krastanov closed this
@Krastanov Krastanov reopened this
@Krastanov
Collaborator

Sorry about the last mail and closing this - I pressed the wrong button.

I just meant to clarify my last comment: I was saying that while I would like to see a nice interactive way to make plots of multiple objects, expand and append should definitely be conserved as they are very basic methods used inside the module. Removing them will require serious refactoring.

@asmeurer
Owner

While on this topic: in this PR the only way to plot, for instance, "parametric 3d line over a 3d surface" or "parametric 2d line and a simple line plot" is to use "Plot.append" and "Plot.extend". In the plot function from master, this was possible by simply supplying the necessary arguments. I see arguments for and against both approaches so I do not have a strong opinion.

The problem is that there really isn't an unambiguous way to differentiate between parametric and non-parametric. I should point out that you can always plot a line in parametric mode by plotting the function vs. x.

@travisbot

This pull request fails (merged 38c4752 into b52b7c3).

@catchmrbharath

@Krastanov I have been trying to get check_arguments to be smaller(more like plot), but I don't see any places where I can further refractor unless I use a list for multiple plots. @asmeurer was against using list and tuples to distinguish between arguments. Also,
plot_line(x, x**2) and plot_parametric( (cos(x), sin(x)), (cos(x), x) ) looks more intuitive than plot_line([x, x**2]).

@Krastanov
Collaborator

You are probably right. The only issue that worries me now is what I have said about the docs.

@asmeurer
Owner

@catchmrbharath will you be able to finish this soon? I'd like to be able to get the new API in before the release.

@catchmrbharath

Yeah I will finish it off today.

catchmrbharath added some commits
@catchmrbharath catchmrbharath Documentation improvements. 1631162
@catchmrbharath catchmrbharath Removed the plot function
1) The set_item cannot take plot arguments as input
2) Overloaded Series Factory is deleted.
866f93f
@catchmrbharath

@Krastanov Can you look at it once again? I have made some changes.
Thanks.

@travisbot

This pull request fails (merged 1631162 into b52b7c3).

@travisbot

This pull request fails (merged 866f93f into b52b7c3).

@travisbot

This pull request fails (merged 4e86389 into b52b7c3).

@travisbot

This pull request fails (merged a89958c into b52b7c3).

@catchmrbharath catchmrbharath plotting docstrings added to the sphinx
* changes to comments so that they look nice on sphinx.
786fb8b
@catchmrbharath

@Krastanov Wont the names you suggested for 3D plots be too long?
@asmeurer I have added the docs to sphinx. Can you just look at it, and see whether I have done it right. Thanks.

@travisbot

This pull request passes (merged 786fb8b into b52b7c3).

@asmeurer
Owner

There are some serious doctest failures here (pasting here in case I have to kill sympy-bot):

examples/advanced/fem.py[1] .                                               [OK]
examples/advanced/gibbs_phenomenon.py[2] ..                                 [OK]
Traceback (most recent call last):
  File "setup.py", line 280, in <module>
    classifiers = classifiers,
  File "/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/distutils/core.py", line 152, in setup
    dist.run_commands()
  File "/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/distutils/dist.py", line 975, in run_commands
    self.run_command(cmd)
  File "/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/distutils/dist.py", line 995, in run_command
    cmd_obj.run()
  File "setup.py", line 173, in run
    sympy.utilities.runtests.run_all_tests()
  File "/private/var/folders/wc/dppcpmxs1tlb36nqcw853wkm0000gq/T/sympy-bot-tmpFUVigL/sympy/sympy/utilities/runtests.py", line 249, in run_all_tests
    if not doctest(*doctest_args, **doctest_kwargs):
  File "/private/var/folders/wc/dppcpmxs1tlb36nqcw853wkm0000gq/T/sympy-bot-tmpFUVigL/sympy/sympy/utilities/runtests.py", line 502, in doctest
    return not bool(_doctest(*paths, **kwargs))
  File "/private/var/folders/wc/dppcpmxs1tlb36nqcw853wkm0000gq/T/sympy-bot-tmpFUVigL/sympy/sympy/utilities/runtests.py", line 567, in _doctest
    failed = not t.test()
  File "/private/var/folders/wc/dppcpmxs1tlb36nqcw853wkm0000gq/T/sympy-bot-tmpFUVigL/sympy/sympy/utilities/runtests.py", line 963, in test
    self.test_file(f)
  File "/private/var/folders/wc/dppcpmxs1tlb36nqcw853wkm0000gq/T/sympy-bot-tmpFUVigL/sympy/sympy/utilities/runtests.py", line 986, in test_file
    module = pdoctest._normalize_module(module)
  File "/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/doctest.py", line 203, in _normalize_module
    return __import__(module, globals(), locals(), ["*"])
  File "/private/var/folders/wc/dppcpmxs1tlb36nqcw853wkm0000gq/t/sympy-bot-tmpfuvigl/sympy/examples/beginner/plot_examples.py", line 15, in <module>
    b = plot(expr, (x, 2, 4), show=False) # cartesian plot
TypeError: 'module' object is not callable
@Krastanov

If this was mentioned in docstrings it should get removed from them as well.

@Krastanov Krastanov commented on the diff
doc/src/modules/plotting.rst
@@ -1,3 +1,82 @@
+Plotting Module
+===============
+
+.. module:: sympy.plotting.plot
+
+Introduction
+------------
+
+The plotting module allows you to plot 2-dimensional and 3-dimensional plots.
+Presently the plots are rendered using ``Matplotlib`` as its backend. It is
+also possible to plot 2-dimensional plots using a ``TextBackend`` if you don't
+have ``Matplotlib``.
+
+The plotting module has the following functions:
@Krastanov Collaborator

Maybe say something about the fact that these functions are only for convenience and that the module is build around Series and Plot objects that can be called directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Krastanov Krastanov commented on the diff
sympy/plotting/plot.py
@@ -1234,3 +1041,614 @@ def _matplotlib_list(interval_list):
xlist.extend([None, None, None, None])
ylist.extend([None, None, None, None])
return xlist, ylist
+
+
+####New API for plotting module ####
+
+# TODO: Add color arrays for plots.
+# TODO: Add more plotting options for 3d plots.
+# TODO: Adaptive sampling for 3D plots.
@Krastanov Collaborator

todo: contour plots (everything is already there, you can just copy/paste your new plot3d function and change the Series class)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
sympy/plotting/plot.py
((181 lines not shown))
+
+ Arguments for ``Parametric2DLineSeries`` class:
+
+ ``adaptive``: Boolean. The default value is set to True. Set adaptive to
+ False and specify ``nb_of_points`` if uniform sampling is required.
+
+ ``depth``: int Recursion depth of the adaptive algorithm. A depth of
+ value ``n`` samples a maximum of `2^{n}` points.
+
+ ``nb_of_points``: int. Used when the ``adaptive`` is set to False. The
+ function is uniformly sampled at ``nb_of_point`` number of points.
+
+ Aesthetics
+ ----------
+
+ ``line_color``: float. Specifies the color for the plot.
@Krastanov Collaborator

It is not a float, it is a function returning a float. Check the "coloring" example notebook. Another docstring already explains all this, just add in parentheses a reference to it (either Plot or Series).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Krastanov
Collaborator

I think that there is a number of example notebooks that are still using the now nonexistent plot function.

The name that I suggested may indeed turn out to be too long, however I still find the present ones a bit confusing (maybe it is just be :)

Besides some minor corrections to the docstrings I think that they should be sufficient.

@travisbot

This pull request passes (merged 6bfa913 into b52b7c3).

catchmrbharath added some commits
@catchmrbharath catchmrbharath Renamed the plot functions
* plot_line -> plot
* plot3D -> plot3d
* plot3D_parametric -> plot3d_parametric_line
* plot3D_surface -> plot3d_parametric_surface
e294832
@catchmrbharath catchmrbharath changed the renamed functions 4560077
@catchmrbharath catchmrbharath More documentation changes 41466ca
@catchmrbharath

I have changed the plot names.

  • plot_line -> plot
  • plot3D -> plot3d
  • plot3D_parametric -> plot3d_parametric_line
  • plot3D_surface -> plot3d_parametric_surface

@asmeurer I have also fixed the doctest failures and addressed @Krastanov comments.

@travisbot

This pull request fails (merged 41466ca into b52b7c3).

@Krastanov
Collaborator

I do not find any obvious issues anymore. Thanks for all the work.

@asmeurer
Owner

By the way, @catchmrbharath as noted on the mailing list, please refrain from rebasing this branch, so it can be merged into 0.7.2.

@asmeurer
Owner

SymPy Bot Summary: :red_circle: Failed after merging catchmrbharath/restructured_plot (41466ca) into master (a184841).
@catchmrbharath: Please fix the test failures.
:red_circle: Python 2.5.0-final-0: fail
:red_circle: Python 2.6.6-final-0: fail
:red_circle: Python 2.7.2-final-0: fail
:red_circle: Python 2.6.8-final-0: fail
:red_circle: Python 2.7.3-final-0: fail
:red_circle: PyPy 1.9.0-final-0; 2.7.2-final-42: fail
:eight_spoked_asterisk: Python 3.2.2-final-0: pass
:eight_spoked_asterisk: Python 3.3.0-beta-2: pass
:eight_spoked_asterisk: Python 3.2.3-final-0: pass
:eight_spoked_asterisk: Python 3.3.0-beta-2: pass
:red_circle:Sphinx 1.1.3: fail

@catchmrbharath

Sorry about that. Forgot to run quality test for the last commit. I will squash the new commit once all tests pass.

@catchmrbharath

I am not sure why the sphinx make is failing. It runs without a problem on my machine.

@asmeurer
Owner

I think it is a failure from master.

@asmeurer
Owner

I'm running the tests against 0.7.2 to see how things will be there.

@travisbot

This pull request passes (merged 274ec3f into b52b7c3).

@asmeurer
Owner

SymPy Bot Summary: :red_circle: Failed after merging catchmrbharath/restructured_plot (274ec3f) into origin/0.7.2 (c1c3b17).
@catchmrbharath: Please fix the test failures.
:eight_spoked_asterisk: Python 2.5.0-final-0: pass
:eight_spoked_asterisk: Python 2.6.6-final-0: pass
:eight_spoked_asterisk: Python 2.7.2-final-0: pass
:eight_spoked_asterisk: Python 2.6.8-final-0: pass
:eight_spoked_asterisk: Python 2.7.3-final-0: pass
:red_circle: PyPy 1.9.0-final-0; 2.7.2-final-42: fail
:eight_spoked_asterisk: Python 3.2.2-final-0: pass
:eight_spoked_asterisk: Python 3.3.0-beta-2: pass
:eight_spoked_asterisk: Python 3.2.3-final-0: pass
:eight_spoked_asterisk: Python 3.3.0-beta-2: pass
:red_circle:Sphinx 1.1.3: fail

@asmeurer
Owner

OK, those Sphinx errors you do need to fix.

@catchmrbharath

SymPy Bot Summary: :eight_spoked_asterisk: All tests have passed.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY6eYkDA

Interpreter: /usr/local/bin/python (2.7.3-final-0)
Architecture: Darwin (64-bit)
Cache: yes
Test command: setup.py test
master hash: a184841
branch hash: 274ec3f

Automatic review by SymPy Bot.

@asmeurer
Owner

SymPy Bot Summary: :red_circle: Failed after merging catchmrbharath/restructured_plot (6478b79) into origin/0.7.2 (c1c3b17).
@catchmrbharath: Please fix the test failures.
:red_circle:Sphinx 1.1.3: fail

@asmeurer
Owner

SymPy Bot Summary: :red_circle: Failed after merging catchmrbharath/restructured_plot (6478b79) into HEAD (6478b79).
@catchmrbharath: Please fix the test failures.
:red_circle:Sphinx 1.1.3: fail

@catchmrbharath

SymPy Bot Summary: :red_circle: Failed after merging catchmrbharath/restructured_plot (6478b79) into master (a184841).
@catchmrbharath: Please fix the test failures.
:red_circle:Sphinx 1.1.3: fail

@catchmrbharath

SymPy Bot Summary: :red_circle: Failed after merging catchmrbharath/restructured_plot (6478b79) into master (a184841).
@catchmrbharath: Please fix the test failures.
:red_circle:Sphinx 1.1.3: fail

@asmeurer asmeurer merged commit 5b149c2 into from
@coveralls

Coverage Status

Changes Unknown when pulling 6478b79 on catchmrbharath:restructured_plot into ** on sympy:master**.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 15, 2012
  1. @catchmrbharath
  2. @catchmrbharath
  3. @catchmrbharath
  4. @catchmrbharath

    New plot functions added

    catchmrbharath authored
    * ``plot_line`` plots 2D lines
    * ``plot_parametric`` plots 2D parametric lines
    * ``plot3D`` plots 3D plots of expression of 2 variables
    * ``plot3D_parametric`` plots 3D parametric line plots
    * ``plot3D_surface`` plots 3D surfaces defined by 2 parameters
  5. @catchmrbharath

    Documentation changes

    catchmrbharath authored
    * Addressed the comments
    * Added information about which keyword arguments belong to which
    class
  6. @catchmrbharath

    Documentation changes.

    catchmrbharath authored
    * Addressed the comments.
    * Add more information about default range.
  7. @catchmrbharath

    Added support for depth and nb_of_points

    catchmrbharath authored
    * depth - recursion depth for adaptive sampling
    * adaptive - set to false, if uniform sampling is needed
    * nb_of_points - number of points the range has to be uniformly sampled into.
    
    Changes to the documentation
    Removed the parameters that are not defined for 3D plots.
  8. @catchmrbharath

    added tests for new plotting functions

    catchmrbharath authored
    * added a global variable _show in plot module, if set to False
    doesn't plot the graph, by default. This can be used in tests
    where we have just save the plots without showing.
    
    * changed tests so that they use the new plotting functions.
  9. @catchmrbharath

    Changed the notebook to use the new notebook.

    catchmrbharath authored
    * Removed all instances of plot
    * Added examples for each plot_* function
  10. @catchmrbharath
  11. @catchmrbharath
  12. @catchmrbharath
  13. @catchmrbharath

    replace ``plot`` with the new functions

    catchmrbharath authored
    * Also removed the ``nsolve`` example which was failing.
  14. @catchmrbharath

    implicit plot tests with show unset

    catchmrbharath authored
    Also fixed a problem with xlim
Commits on Aug 16, 2012
  1. @catchmrbharath
Commits on Aug 23, 2012
  1. @catchmrbharath
  2. @catchmrbharath

    Removed the plot function

    catchmrbharath authored
    1) The set_item cannot take plot arguments as input
    2) Overloaded Series Factory is deleted.
  3. @catchmrbharath

    doctests fixes

    catchmrbharath authored
  4. @catchmrbharath
  5. @catchmrbharath
  6. @catchmrbharath
  7. @catchmrbharath

    plotting docstrings added to the sphinx

    catchmrbharath authored
    * changes to comments so that they look nice on sphinx.
Commits on Aug 24, 2012
  1. @catchmrbharath
  2. @catchmrbharath
  3. @catchmrbharath

    Renamed the plot functions

    catchmrbharath authored
    * plot_line -> plot
    * plot3D -> plot3d
    * plot3D_parametric -> plot3d_parametric_line
    * plot3D_surface -> plot3d_parametric_surface
  4. @catchmrbharath
  5. @catchmrbharath
  6. @catchmrbharath

    fixed trailing space

    catchmrbharath authored
Commits on Aug 25, 2012
  1. @catchmrbharath

    sphinx fixes

    catchmrbharath authored
Something went wrong with that request. Please try again.