sagemath / sage

Main repository of SageMath. Now open for Issues and Pull Requests.
https://www.sagemath.org
Other
1.19k stars 411 forks source link

Regression in riemann.pyx #17387

Open vbraun opened 9 years ago

vbraun commented 9 years ago

The following comes from a doctest in src/sage/calculus/riemann.pyx:

Sage 5.12:

sage: f(t) = e^(I*t) - 0.5*e^(-I*t)
sage: fprime(t) = I*e^(I*t) + 0.5*I*e^(-I*t)
sage: m = Riemann_Map([f], [fprime], 0)
sage: %time m.plot_colored(plot_points=1000).save("/tmp/foo.png")
CPU times: user 33.13 s, sys: 0.78 s, total: 33.92 s
Wall time: 34.73 s

Sage 6.3:

sage: f(t) = e^(I*t) - 0.5*e^(-I*t)
sage: fprime(t) = I*e^(I*t) + 0.5*I*e^(-I*t)
sage: m = Riemann_Map([f], [fprime], 0)
sage: %time m.plot_colored(plot_points=1000).save("/tmp/foo.png")
CPU times: user 1min 6s, sys: 1.81 s, total: 1min 7s
Wall time: 1min 8s

Component: graphics

Branch/Commit: u/vbraun/regression_in_riemann_pyx @ 60272e7

Issue created by migration from https://trac.sagemath.org/ticket/17387

jdemeyer commented 9 years ago
comment:1

Is it a regression? If plotting did become slower, then it's something to be investigated.

The comment mentions that it takes 29s on sage.math which is not a very fast machine. Other doctests are documented to take far more time. This 29s shouldn't cause timeouts (unless the machine is very slow, but then you should just increase the timeout).

vbraun commented 9 years ago
comment:2

I don't think its a regression, I think I've seen it before. Its just unnecessarily slow.

vbraun commented 9 years ago
comment:3

PS: Machine is my desktop, Haswell-E 6-core with 32 GB ram (but doing stuff in the background).

jdemeyer commented 9 years ago

Description changed:

--- 
+++ 
@@ -1,7 +1,23 @@
+The following comes from a doctest in `src/sage/calculus/riemann.pyx`:
+
+Sage 5.12:

-sage: m.plot_colored(plot_points=1000) # long time (29s on sage.math, 2012) ## line 1051 ## -[...] -sage -t --long src/sage/calculus/riemann.pyx # Timed out +sage: f(t) = e^(It) - 0.5e^(-It) +sage: fprime(t) = Ie^(It) + 0.5Ie^(-It) +sage: m = Riemann_Map([f], [fprime], 0) +sage: %time m.plot_colored(plot_points=1000).save("/tmp/foo.png") +CPU times: user 33.13 s, sys: 0.78 s, total: 33.92 s +Wall time: 34.73 s

-Do we really test an independent code path by computing extra many points?
+
+Sage 6.3:
+
+```
+sage: f(t) = e^(I*t) - 0.5*e^(-I*t)
+sage: fprime(t) = I*e^(I*t) + 0.5*I*e^(-I*t)
+sage: m = Riemann_Map([f], [fprime], 0)
+sage: %time m.plot_colored(plot_points=1000).save("/tmp/foo.png")
+CPU times: user 1min 14s, sys: 3.77 s, total: 1min 17s
+Wall time: 1min 31s
+```
jdemeyer commented 9 years ago

Description changed:

--- 
+++ 
@@ -18,6 +18,6 @@
 sage: fprime(t) = I*e^(I*t) + 0.5*I*e^(-I*t)
 sage: m = Riemann_Map([f], [fprime], 0)
 sage: %time m.plot_colored(plot_points=1000).save("/tmp/foo.png")
-CPU times: user 1min 14s, sys: 3.77 s, total: 1min 17s
-Wall time: 1min 31s
+CPU times: user 1min 6s, sys: 1.81 s, total: 1min 7s
+Wall time: 1min 8s
vbraun commented 9 years ago

Branch: u/vbraun/regression_in_riemann_pyx

kcrisman commented 9 years ago
comment:7

This is probably due to some great final stuff Ethan put in that I finally reviewed not that long ago (in Sage time). Comments:

p = m.plot_spiderweb(withcolor=True, plot_points=100, thickness = 2.0, min_mag=0.1) # long time

is just not acceptable, it's completely unintelligible. plot_points=350 is the first time it seems reasonable.


New commits:

60272e7Waste less time in riemann.pyx plots
kcrisman commented 9 years ago

Commit: 60272e7

jdemeyer commented 9 years ago
comment:8

The main goal of this ticket should be to fix the actual regression in the plotting. Once that's done, we can have a look at what should be done about the doctests.

kcrisman commented 9 years ago
comment:9

Okay, #11273 was merged in Sage 5.13.beta0 (thanks, "Merged in" box!) - probably commit f597e49695b. The longer plot test time overall is probably mostly due to additional tests being added for quite a bit of new functionality (multiply connected domains, etc.). The specific example in question is most likely due to moving the generation of plot points (the thing that actually is slow), not the image creation itself) to compute_on_grid, though I didn't see any obviously wrong or new code at the time, nor do I see anything obvious where Cython was replaced with Python there.

By the way, on my OS X 10.7 2.3 GHz laptop with only 8GB, I still get about 35 seconds like in your earlier version.

jdemeyer commented 8 years ago

Changed keywords from random_fail to none