remove duplicate args in @options() in region_plot #36296
Conversation
This is needed for Sphinx 7.1+
|
As I understand it (from reading examples), the That is the theory. You should test. |
|
There seems to be no documentation on such a use of and there is no example of the use of So it appears that we can just as well write def g(f,a=42):
print(a)to almost the same effect (the only difference is that we still can call |
|
I think if you want to object to this change, you'd have to come up with a meaningful doctest which shows that the proposed change breaks something. |
|
Calling the original author of these lines: @jhonrubiagonzalez Also, it seems @vbraun touched some People, what was it meant to do?! |
|
Anyhow, I have built the Sage docs using Sphinx 7.2.6, and I can see the difference in picture quality as the number of |
|
It is used like this: This is certainly very useful, especially for plotting functions which typically have lots of options. You can set the defaults for options as you want, and the subsequent function calls would get those defaults. |
|
A proper fix for problems including the present one with Sphinx 7.1+ would involve updating (or better removing) |
|
what does this particular bug have to do with autodoc? |
|
The present branch allows overriding the defaults which are not listed in |
|
Then you can get rid of all options listed in |
Just guess. Sphinx autodoc has to do with extracting function signature to document the function. We can suspect that the new Sphinx has a problem with dealing with the decorated function... I have no exact idea. |
If so, why not just put everything on either |
|
Note that |
|
I am experimenting with |
|
After looking at the code, I think there's no difference in behavior wherever arguments are put. One difference is the documentation. Seeing the output of In this point of view, you should put as many arguments to Hence, after all, removing options from It seems to me that this is a slight price for getting Sphinx 7.1+, so I am okay with your change if you add a comment there about why and what options were removed from the list in what PR. |
That's lucky, I didn't think the behavior would be the same.
The default return implicit_plot(feqs[0], xrange, yrange, plot_points=plot_points,
fill=False, linewidth=borderwidth,
linestyle=borderstyle, color=bordercol, **options)g._set_extra_kwds(Graphics._extract_kwds_for_show(options, ignore=['xmin', 'xmax']))g.add_primitive(ContourPlot(xy_data_array, xrange, yrange,
dict(contours=[-1e-20, 0, 1e-20],
cmap=cmap,
fill=True, **options))) |
I think putting them in |
|
I agree. I didn't know it exists.
|
|
I searched for the options decorator. It is used extensively in |
Something like .01% of the functions that take options though. |
|
Back to the original issue, removing duplicates from the function arguments list, instead of |
That catches all keyword arguments from I meant |
This would break, potentially, the order of the positional arguments of the function. It might be OK though. I'll check. |
I fully agree. Having some of the arguments of Side comment: fixing this will probably fix the missing link to |
I tried this, it just doesn't work. One has to understand how that blasted incomprehensible errors if I do +++ b/src/sage/plot/contour_plot.py
@@ -1392,10 +1392,9 @@ def implicit_plot(f, xrange, yrange, **options):
@options(plot_points=100, incol='blue', outcol=None, bordercol=None,
- borderstyle=None, borderwidth=None, frame=False, axes=True,
- legend_label=None, aspect_ratio=1, alpha=1)
-def region_plot(f, xrange, yrange, plot_points, incol, outcol, bordercol,
- borderstyle, borderwidth, alpha, **options):
+ borderstyle=None, borderwidth=None, alpha=1,
+ frame=False, axes=True, legend_label=None, aspect_ratio=1)
+def region_plot(f, xrange, yrange, **options):
r"""
``region_plot`` takes a boolean function of two variables, `f(x, y)`
and plots the region where f is True over the specified
@@ -1659,6 +1658,14 @@ def region_plot(f, xrange, yrange, plot_points, incol, outcol, bordercol,
from warnings import warn
import numpy
+ plot_points = options['plot_points']
+ incol = options['incol']
+ outcol = options['outcol']
+ bordercol = options['bordercol']
+ borderstyle = options['borderstyle']
+ borderwidth = options['borderwidth']
+ alpha = options['alpha']
+
if not isinstance(f, (list, tuple)):
f = [f]
...
*********************************************************************
File "src/sage/plot/contour_plot.py", line 1650, in sage.plot.contour_plot.region_plot
Failed example:
region_plot([x == 0], (x,-1,1), (y,-1,1))
Exception raised:
Traceback (most recent call last):
File "/home/scratch/scratch2/dimpase/sage/sage/src/sage/doctest/forker.py", line 709, in _run
self.compile_and_execute(example, compiler, test.globs)
File "/home/scratch/scratch2/dimpase/sage/sage/src/sage/doctest/forker.py", line 1144, in compile_and_execute
exec(compiled, globs)
File "<doctest sage.plot.contour_plot.region_plot[21]>", line 1, in <module>
region_plot([x == Integer(0)], (x,-Integer(1),Integer(1)), (y,-Integer(1),Integer(1)))
File "/home/scratch/scratch2/dimpase/sage/sage/src/sage/misc/decorators.py", line 497, in wrapper
return func(*args, **options)
^^^^^^^^^^^^^^^^^^^^^^
File "/home/scratch/scratch2/dimpase/sage/sage/src/sage/plot/contour_plot.py", line 1687, in region_plot
return implicit_plot(feqs[0], xrange, yrange, plot_points=plot_points,
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: sage.plot.contour_plot.implicit_plot() got multiple values for keyword argument 'plot_points'
...and if I leave out assignments in the function body I get errors about undefined local variables: Unless we dig up someone who knows about |
|
The code in the branch |
OK, but why you don't pop (these are exactly the options with names not matching names of formal args of In your branch, after all the option popping, you'd still have |
|
@egourgoulhon : Another problem is that your branch changes the number of positional args of How about we do my changes for the time being, and deprecate |
There reason for keeping |
The current definition of |
|
The real positional arguments are only Let's get his code in. No deprecation is needed. |
The current code does not pass it. And your code does pass it, right? |
…ns_fix_in_contour_plot This also fixes a bug in region_plot, so that plot_points are not passed in calls to other functions via options. This is because a @options decorator's argument are automatically popped from "options" dict as soon as the function it decorates has an arg with the same name. In the change we reduce the number of positional arguments of region_plot; thus the corresponding args of @options have to be popped manually in the function body; we don't pop plot_points, to pass it correctly in function calls.
|
OK, I've taken @egourgoulhon commit, and added a longer comment in the merge commit. |
Right. But it seems legitimate to do so, because |
After regenerating the full doc, I confirm that this issue is fixed by the commit 80fd0a9 |
|
Also, comparing with the naked eye all the plots of Sage 10.1 documentation at |
This also fixes the missing |
|
Documentation preview for this PR (built with commit b7c6367; changes) is ready! 🎉 |
|
It looks very good. Thanks everyone! |
This is a continuation of sagemath#36256 - Fixes sagemath#36301 - Fixes https://groups.google.com/g/sage- release/c/1wOBmhvNJqc/m/Jk14VAbjBAAJ (hence marked critical) ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies - sagemath#36296: to use an up to date Sphinx - sagemath#36267: updated ipympl, etc Also, a Jupyter/Python issue was uncovered there, which might need work. URL: sagemath#36276 Reported by: Dima Pasechnik Reviewer(s): Matthias Köppe, Michael Orlitzky
This is a continuation of sagemath#36256 - Fixes sagemath#36301 - Fixes https://groups.google.com/g/sage- release/c/1wOBmhvNJqc/m/Jk14VAbjBAAJ (hence marked critical) ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies - sagemath#36296: to use an up to date Sphinx - sagemath#36267: updated ipympl, etc Also, a Jupyter/Python issue was uncovered there, which might need work. URL: sagemath#36276 Reported by: Dima Pasechnik Reviewer(s): Matthias Köppe, Michael Orlitzky
This is needed for Sphinx 7.1+, as outlined in #36295 and discovered in #36276
This will resolve #36295
It also fixes a bug in
region_plot, where in some casesplot_pointswere not passed to the backend.