Opened 9 years ago

Closed 8 years ago

#8867 closed enhancement (fixed)

speed up the riemann mapping functionality

Reported by: jason Owned by:
Priority: minor Milestone: sage-4.7.1
Component: calculus Keywords:
Cc: evanandel, kcrisman Merged in: sage-4.7.1.alpha2
Authors: Jason Grout, Ryan Grout Reviewers: Marshall Hampton
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by kcrisman)

This patch speeds up the riemann mapping functionality by automatically trying to call fast_callable on the functions passed in. 


Apply trac-8867-riemann-fastcallable.2.patch. Depends on #10792 and #10821.

I believe that it is no longer true that this depends on #5572 (patch "improve_fast_callable.patch")

Attachments (2)

trac-8867-riemann-fastcallable.patch (15.4 KB) - added by jason 9 years ago.
trac-8867-riemann-fastcallable.2.patch (15.4 KB) - added by evanandel 9 years ago.
Updated to dodge lambdas and work with new doctests

Download all attachments as: .zip

Change History (25)

Changed 9 years ago by jason

comment:1 Changed 9 years ago by jason

  • Cc evanandel added
  • Status changed from new to needs_work

comment:2 Changed 9 years ago by jason

The doctests are *much* faster now; I hope I didn't introduce any bugs! The patch isn't finished yet, I don't think.

comment:3 Changed 9 years ago by jason

(I don't think I can work on this a lot more, but at least this is a start to making this faster)

This is very timely---another faculty member was just asking me a few weeks ago about how to use Sage to visualize Riemann mappings. That's why I thought I'd work on it for a few minutes to speed it up.

comment:4 Changed 9 years ago by evanandel

Excellent Jason, thanks. Let me know if there's anything else that would help you guys.

comment:5 Changed 9 years ago by evanandel

  • Description modified (diff)

What is the status of this patch? I'm working on speeding up the Riemann as a whole, and this would be a good component to have. I see that it depends on the main fast_callable patch

comment:6 Changed 9 years ago by evanandel

  • Description modified (diff)

Whoops, changed description on accident, comment should have read:

What is the status of this patch? I'm working on speeding up the Riemann as a whole, and this would be a good component to have. I see that it depends on the main fast_callable patch, do you have any idea when that is expected to finish?

Not at all trying to rush you, just curious for my own purposes if this is coming through soon.

comment:7 Changed 9 years ago by jason

No guarantees, but I'd like to finish the fast_callable patch over Christmas break (so before the middle of January).

comment:8 Changed 9 years ago by jason

Actually, I just tried applying this patch without #5572 and things seem to work just fine. You might try testing this patch anyway and reviewing it.

comment:9 Changed 9 years ago by jason

  • Authors set to Jason Grout

So I don't think this depends on #5572 anymore.

comment:10 Changed 9 years ago by evanandel

Did all of the tests work for you? The riemann tests go fine, but the interpolators do this:

File "/home/ethan/sage-4.5.3/devel/sage/sage/calculus/interpolators.pyx", line 52:

  sage: m = Riemann_Map([lambda x: ps.value(real(x))], [lambda x: ps.derivative(real(x))],0)

Exception raised:

  Traceback (most recent call last):
    File "/home/ethan/sage-4.5.3/local/bin/ncadoctest.py", line 1231, in run_one_test
      self.run_one_example(test, example, filename, compileflags)
    File "/home/ethan/sage-4.5.3/local/bin/sagedoctest.py", line 38, in run_one_example
      OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
    File "/home/ethan/sage-4.5.3/local/bin/ncadoctest.py", line 1172, in run_one_example
      compileflags, 1) in test.globs
    File "<doctest __main__.example_1[7]>", line 1, in <module>
      m = Riemann_Map([lambda x: ps.value(real(x))], [lambda x: ps.derivative(real(x))],Integer(0))###line 52:
  sage: m = Riemann_Map([lambda x: ps.value(real(x))], [lambda x: ps.derivative(real(x))],0)
    File "riemann.pyx", line 164, in sage.calculus.riemann.Riemann_Map.__init__ (sage/calculus/riemann.c:1443) File "fast_callable.pyx", line 399, in sage.ext.fast_callable.fast_callable (sage/ext/fast_callable.c:2668)
  AttributeError: 'function' object has no attribute 'variables'

I can solve this by wrapping the fast-callable casts in a try except block, but of course that means that it isn't using them for unusual functions like the interpolators.

comment:11 Changed 9 years ago by jason

fast_callable won't do anything to speed up a lambda function anyway. It just blindly wraps them in a python call. So I don't think there's any loss in having a try-except block.

comment:12 Changed 9 years ago by jason

My big patch to fast_callable should also be able to handle the error, but it probably won't be finished or ready for testing for at least another couple of weeks, so if you have time now, don't wait for it.

Changed 9 years ago by evanandel

Updated to dodge lambdas and work with new doctests

comment:13 Changed 9 years ago by evanandel

  • Description modified (diff)
  • Status changed from needs_work to needs_review

I've uploaded a new version of the fast_callable patch. It now properly avoids failing with lambda functions, although it doesn't work optimally for them. I'll add some notes on that in #10945. It also is updated to work properly with the numpy 1.5.1 version. Therefore tickets #10792 and #10821 should be applied first. I don't think the current patch is dependent on #5572, although I'm sure that faster fast_callables would help.

comment:14 Changed 8 years ago by kcrisman

  • Cc kcrisman added

comment:15 Changed 8 years ago by kcrisman

  • Authors changed from Jason Grout to Jason Grout, Ryan Grout
  • Description modified (diff)

comment:16 Changed 8 years ago by mhampton

  • Owner changed from burcin to (none)

This seems to be faster and less buggy than before, although I still have a pretty easy time tripping it up. For example, the following crescent region causes problems if its not translated as I do below, but even when its translated so that 0 is in the interior the spiderweb plot looks wrong:

npi = N(pi)
crescent = [(cos(t)+.9,sin(t)) for t in srange(npi/2,3*npi/2,npi/12)]
crescent = crescent + [(5.0*cos(t)/6+.9,sin(t)) for t in srange(3*npi/2,npi/2,-npi/12)]
ps = polygon_spline(crescent) 
f = lambda t: ps.value(real(t)) 
fprime = lambda t: ps.derivative(real(t)) 
m = Riemann_Map([f], [fprime], 0.25, ncorners=24) 
show(m.plot_colored() + m.plot_spiderweb(pts=100),figsize=[6,6])

But maybe that sort of problem should be in a seperate ticket.

comment:17 Changed 8 years ago by kcrisman

Yeah, if you think this is good to go (i.e., correct and doesn't make functionality worse), you can give it positive review. If you can open a ticket with things like this plot and the untranslated one, that would be great. It sounds like Ethan is working on it a fair amount right now (?), so it would be good to have it on the burner.

comment:18 Changed 8 years ago by evanandel

The example given is naturally a problematic one. With the center located outside (a = .25) of the region, it is mathematically nonsensical, thus any output that you get will be bizarre. For the a = 0 case, the spiderweb plot is indeed slightly erratic (caused by the natural inaccuracy of the numerical integration near the boundary). This can be solved by decreasing the linescale parameter. This is documented, but I can see now that it isn't quite clear enough. I'll elaborate as part of the major documentation changes described in #10945.

I'm curious, what were you trying to accomplish by setting ncorners to 24 when this region only has 2? Is this poorly documented?

To elaborate, none of the behavior seen in this example is incorrect. Let me know if there are any other doc changes I can make to make this easier (I'm working on that already).

comment:19 Changed 8 years ago by mhampton

  • Status changed from needs_review to positive_review

The crescent is a polygon with 24 vertices, so I thought using ncorners=24 would make sense.

To be honest, I didn't read the documentation very carefully. This is a good simulation of general users :) Now that I have, I am happy with the behavior of this function.

comment:20 Changed 8 years ago by evanandel

Ah, good point. I'll clarify that in the docs.

comment:21 Changed 8 years ago by evanandel

Specifically in the major revision I'm working on, not here.

comment:22 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-4.7 to sage-4.7.1
  • Reviewers set to Marshall Hampton

comment:23 Changed 8 years ago by jdemeyer

  • Merged in set to sage-4.7.1.alpha2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.