-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Deprecating markers, annotations, fill, rectangles properties of the Plot class #25117
Conversation
…Plot class The properties ``markers, annotations, fill, rectangles`` (containing user-provided numerical data to be added on a plot) are deprecated. The new implementation saves user-provided numerical data into a new data series, `GenericSeries`, which can easily be processed by ``MatplotlibBackend``. This means that user-provided numerical data is also taken into consideration when combining multiple plots with the ``extend`` and ``append`` methods. Instead of setting those properties directly, users should pass the homonym keyword arguments to the plotting functions. **Motivation for this deprecation:** the implementation of the ``Plot`` class suggests that it is ok to add attributes and hard-coded if-statements in the ``MatplotlibBackend`` class to provide more and more functionalities for user-provided numerical data (e.g. adding horizontal lines, or vertical lines, or bar plots, etc). However, in doing so one would reinvent the wheel: plotting libraries already implements the necessary API. There is no need to hard code these things. The plotting module should facilitate the visualization of symbolic expressions. The best way to add custom numerical data is to retrieve the figure created by the plotting module and use the API of a particular plotting library. For example: ```py p = plot(cos(x)) fig, ax = p._backend.fig, p._backend.ax[0] ax.plot([0, 1, 2], [0, 1, -1], "*") ax.axhline(0.5) fig ```
✅ Hi, I am the SymPy bot. 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:
This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.13. Click here to see the pull request description that was parsed.
Update The release notes on the wiki have been updated. |
I haven't checked this very closely as I am not too familiar with any potential complex usage of the plotting module (using markers etc already goes beyond my own usage). That being said I think this broadly looks good. The release note should say The deprecation description in the docs could be a little clearer by giving an explicit example of old usage that is now considered deprecated and exactly what that specific example should be replaced by. Also if the suggestion is to do |
Thanks Oscar, great suggestions!
Ok, I was following this deprecation guide which clearly should be updated :) |
# plot symbolic expression | ||
p = plot(cos(x)) | ||
# retrieve Matplotlib's figure and axes object | ||
fig, ax = p._backend.fig, p._backend.ax[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that it is good to encourage users to use implementation details like _backend
though.
Maybe it is better to develop a public function, like to_backend
, and encourage users that if you start to use backend directly, sympy is not responsible with any context you keep working with the backend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, that will come on a future PR!
You need to fix the author tests |
Benchmark results from GitHub Actions Lower numbers are good, higher numbers are bad. A ratio less than 1 Significantly changed benchmark results (PR vs master) Significantly changed benchmark results (master vs previous release) before after ratio
[8059df73] [8cf07e36]
<sympy-1.12^0>
- 85.5±0.4ms 54.7±0.3ms 0.64 integrate.TimeIntegrationRisch02.time_doit(10)
- 83.8±0.3ms 53.5±0.2ms 0.64 integrate.TimeIntegrationRisch02.time_doit_risch(10)
Full benchmark results can be found as artifacts in GitHub Actions |
Looks good to me. |
@Davide-sd I edited the release note. Let me know if it looks incorrect. This looks good so I'll merge it. |
Looks good, thank you! |
References to other Issues or PRs
NOTEs:
Brief description of what is fixed or changed
The properties
markers, annotations, fill, rectangles
of thePlot
class (containing user-provided numerical data to be added on a plot) are deprecated. The new implementation saves user-provided numerical data into a new data series,GenericDataSeries
, which can easily be processed byMatplotlibBackend
. This makes the properties unnecessary. It also means that user-provided numerical data is taken into consideration when combining multiple plots with theextend
andappend
methods (previously, they were disregarded).Instead of setting those properties directly, users should pass the homonym keyword arguments to the plotting functions.
Motivation for this deprecation: the implementation of the
Plot
class suggests that it is ok to add attributes and hard-coded if-statements in theMatplotlibBackend
class to provide more and more functionalities for user-provided numerical data (e.g. adding horizontal lines, or vertical lines, or bar plots, etc). However, in doing so one would reinvent the wheel: plotting libraries already implement the necessary API. There is no need to hard code these things in this plotting module.The plotting module should facilitate the visualization of symbolic expressions. Then, the best way to add custom numerical data is to retrieve the figure created by the plotting module and use the API of a particular plotting library. For example, with Matplotlib we would write:
Other comments
Release Notes
markers
,annotations
,fill
,rectangles
properties of thePlot
class. These can still be provided as keyword arguments to theplot
function but setting the attributes on the returnedPlot
object is now deprecated.