`sage.plot`: Add/update `# needs` by mkoeppe · Pull Request #36038 · sagemath/sage

mkoeppe · 2023-08-05T22:51:27Z

📝 Checklist

The title is concise, informative, and self-explanatory.
The description explains in detail what this PR is about.
I have linked a relevant issue or discussion.
I have created tests covering the changes.
I have updated the documentation accordingly.

⌛ Dependencies

kwankyu · 2023-08-06T10:48:00Z

src/sage/plot/animate.py

+    ....:                            region=lambda x,y,z: x<=t or y>=t or z<=t)
+    sage: a = animate([frame(t)
+    ....:              for t in srange(.01,1.5,.2)])
+    sage: a[0]                      # long time


L90 looks bad after splitting. Please insert spaces after commas.

I guess I made these changes before deciding to use a file-level tag for sage.symbolic. Undone now in 0754253

src/sage/plot/animate.py

dcoudert

Some questions, and some required changes.

dcoudert · 2023-08-06T10:16:45Z

src/sage/plot/animate.py

+    ....:              for t in srange(.01,1.5,.2)])
+    sage: a[0]                      # long time
    Graphics3d Object
    sage: a.show()                  # long time  # optional -- ImageMagick


Just to be sure, are both forms accepted ? # optional -- ImageMagick and # optional - ImageMagick

All of these forms accepted; sage -fixdoctests normalizes to the # optional - imagemagick form.

dcoudert · 2023-08-06T10:18:36Z

src/sage/plot/animate.py

            sage: print(a)
            Animation with 3 frames
-            sage: a.show()  # optional -- ImageMagick
+            sage: a.show()                      # optional - imagemagick


ImageMagick ?

OK, I am normalizing in this file to # optional -- ImageMagick because that seemed to be the preference of the author

Don't we use uncapitalized names for all optional doctest tags, except this one? If we allow an exception, then the result would just be an inconsistency. I mean that some will use # optional - imagemagick anyway.

Is the author strong about this?

Changing it to the fixdoctests normal form is also fine with me.

@dcoudert ?

We have other exceptions. For instance we use both gurobi and Gurobi:

xyz:sage dcoudert$ git grep "# optional - [gG]urobi" src/sage/ src/sage/combinat/matrices/dancing_links.pyx: sage: d.to_milp('gurobi') # optional - gurobi sage_numerical_backends_gurobi src/sage/combinat/matrices/dancing_links.pyx: sage: s = d.one_solution_using_milp_solver('gurobi') # optional - gurobi sage_numerical_backends_gurobi src/sage/combinat/matrices/dancing_links.pyx: sage: s in solutions # optional - gurobi sage_numerical_backends_gurobi src/sage/features/mip_backends.py: sage: Gurobi()._is_present() # optional - gurobi src/sage/knots/link.py: sage: L.plot(solver='Gurobi') # optional - Gurobi src/sage/numerical/backends/cvxpy_backend.pyx: sage: p = MixedIntegerLinearProgram(solver="CVXPY/Gurobi"); p.solve() # optional - gurobi src/sage/numerical/mip.pyx: sage: p = MixedIntegerLinearProgram(solver="gurobi") # optional - Gurobi src/sage/numerical/mip.pyx: sage: p.add_constraint(p[0] - p[2], min=1, max=4) # optional - Gurobi src/sage/numerical/mip.pyx: sage: p.number_of_variables() # optional - Gurobi

When running git grep "# optional - [[:upper:]]" src/sage/, we see occurrences of: # optional - SAGE_ROOT, # optional - CPLEX, # optional - Gurobi, # optional - Nonexistent_LP_solver, # optional - CHomP, # optional - FES , # optional - RSat.

I don't think we want to force the use of the uncapitalized form for SAGE_ROOT. At least we should be consistent inside a file.

All right then. I won't insist.

src/sage/plot/animate.py

dcoudert · 2023-08-06T10:22:05Z

src/sage/plot/complex_plot.pyx

-        sage: import numpy as np
-        sage: from sage.plot.complex_plot import add_lightness_smoothing_to_rgb
-        sage: add_lightness_smoothing_to_rgb(np.array([[[0, 0.25, 0.5]]]), np.array([[0.75]]))  # abs tol 1e-4
+        sage: import numpy as np                                                        # needs numpy


block scope here

src/sage/plot/complex_plot.pyx

dcoudert · 2023-08-06T10:27:51Z

src/sage/plot/graphics.py

-            sage: p = plot(y, -4.1, 1.1)
-            sage: xlines = lambda a,b: [z for z,m in y.roots()]
-            sage: p.show(gridlines=[xlines, [0]], frame=True, axes=False)
+            sage: y = x^5 + 4*x^4 - 10*x^3 - 40*x^2 + 9*x + 36                          # needs sage.symbolic


block scope

Done in 63dcc78

src/sage/plot/line.py

src/sage/plot/plot.py

dcoudert · 2023-08-06T10:45:28Z

src/sage/plot/plot3d/revolution_plot3d.py

+    - ``trange`` -- A 3-tuple `(t,t_{\min},t_{\max})` where t is the independent variable of the curve.

-    - ``phirange`` - A 2-tuple of the form `(\phi_{\min},\phi_{\max})`, (default `(0,\pi)`) that specifies the angle in which the curve is to be revolved.
+    - ``phirange`` -- A 2-tuple of the form `(\phi_{\min},\phi_{\max})`, (default `(0,\pi)`) that specifies the angle in which the curve is to be revolved.


Could do the alignment on 80 columns

Too much for this ticket

True, it's already a giant one ;)

dcoudert · 2023-08-06T10:46:22Z

src/sage/plot/plot3d/shapes.pyx

-        sage: G += G.translate(2.3, 0, -.5)
-        sage: G += G.translate(3.5, 2, -1)
-        sage: G.show(aspect_ratio=1, frame=False)
+        sage: G += sum(Cylinder(.2, 1).translate(cos(2*pi*n/9), sin(2*pi*n/9), 0)       # needs sage.symbolic


block scope ?

Done in 73a5b51

kwankyu · 2023-08-06T10:58:46Z

src/sage/plot/animate.py

-            sage: B = MyAnimation([graphs.CompleteGraph(n) for n in range(7,11)], figsize=5)
+            sage: B = MyAnimation([graphs.CompleteGraph(n)
+            ....:                  for n in range(7,11)], figsize=5)
            sage: d = B.png()


I am not sure if we should count leading spaces to check 80 characters limit. 80 characters without counting leading spaces look fine in the rendered doc.

OK, I've joined these lines in b5c1e8a

dcoudert · 2023-08-07T09:07:45Z

src/sage/plot/plot.py

    sage: g2 = plot(cos(x), 0, 2*pi, linestyle="--")
-    sage: (g1+g2).show(ticks=pi/6, tick_formatter=pi)  # long time # show their sum, nicely formatted
+    sage: (g1 + g2).show(ticks=pi/6,        # show their sum, nicely formatted  # long time
+....:                    tick_formatter=pi)


The bot reports this alignement error. 4 spaces missing before ....:

Thanks, fixed in e5b62ef

github-actions · 2023-08-07T20:36:26Z

Documentation preview for this PR (built with commit e18852a; changes) is ready! 🎉

dcoudert

LGTM.

mkoeppe · 2023-08-10T17:02:43Z

Thanks!

sagemathgh-36038: `sage.plot`: Add/update `# needs`     - Part of: sagemath#29705 - Cherry-picked from: sagemath#35095  ### 📝 Checklist     - [x] The title is concise, informative, and self-explanatory. - [ ] 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   URL: sagemath#36038 Reported by: Matthias Köppe Reviewer(s): David Coudert, Kwankyu Lee, Matthias Köppe

Matthias Koeppe added 12 commits August 5, 2023 16:35

sage.plot: Add # optional - numpy

645e155

Massive modularization fixes

b78ebc7

sage.ext, sage.plot: More # optional

45115bb

More # optional

caacd0a

src/sage/plot/point.py: Fix up doctest formatting

7352e55

sage.plot: More # optional

59900f2

More # optional

adf8e48

src/sage/plot/graphics.py: Fix doctest

ba4c9e4

Update # optional / # needs

9f6c58c

src/sage/plot: sage -fixdoctests --only-tags, use more block tags

44f3ec5

./sage -fixdoctests --only-tags src/sage/ext/fast_callable.pyx

6ff7f14

src/sage/misc/replace_dot_all.py: Update doctest output

322f19a

kwankyu reviewed Aug 6, 2023

View reviewed changes

src/sage/plot/animate.py Show resolved Hide resolved

kwankyu reviewed Aug 6, 2023

View reviewed changes

src/sage/plot/animate.py Show resolved Hide resolved

kwankyu reviewed Aug 6, 2023

View reviewed changes

src/sage/plot/animate.py Show resolved Hide resolved

dcoudert suggested changes Aug 6, 2023

View reviewed changes

kwankyu reviewed Aug 6, 2023

View reviewed changes

Matthias Koeppe added 7 commits August 6, 2023 18:51

src/sage/plot/complex_plot.pyx: Use block tags

e4e0926

src/sage/plot/animate.py: Reformat

0754253

sage.plot: Use # needs in file-level tags

2326a21

src/sage/plot/graphics.py: Use more block tags

63dcc78

src/sage/plot/{line,plot}.py: Doctest cosmetics

abc8d16

src/sage/plot/plot3d/shapes.pyx: Use block tag

73a5b51

src/sage/plot/animate.py: Reformat

b5c1e8a

dcoudert suggested changes Aug 7, 2023

View reviewed changes

Matthias Koeppe added 2 commits August 7, 2023 09:36

src/sage/plot/plot.py: Fix up docstring markup

e5b62ef

src/sage/plot/plot3d/list_plot3d.py: Fix doc markup

e18852a

dcoudert approved these changes Aug 8, 2023

View reviewed changes

dcoudert added s: positive review and removed s: needs review labels Aug 10, 2023

vbraun merged commit 47b6151 into sagemath:develop Aug 13, 2023

github-actions bot removed the s: positive review label Aug 13, 2023

Uh oh!

Comments

Conversation

mkoeppe commented Aug 5, 2023

📝 Checklist

⌛ Dependencies

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkoeppe Aug 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dcoudert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kwankyu Aug 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 7, 2023

Uh oh!

dcoudert left a comment

Choose a reason for hiding this comment

Uh oh!

mkoeppe commented Aug 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

mkoeppe Aug 7, 2023 •

edited

Loading

kwankyu Aug 7, 2023 •

edited

Loading