Opened 9 years ago

Closed 8 years ago

#10945 closed defect (duplicate)

Fix lots of minor docs and redundancy for riemann.pyx

Reported by: kcrisman Owned by: burcin
Priority: minor Milestone: sage-duplicate/invalid/wontfix
Component: calculus Keywords: riemann map complex plot
Cc: evanandel, jason, wdj, mvngu Merged in:
Authors: Reviewers: Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by kcrisman)

The Riemann mapping stuff is a great addition - excellent work!

But there are lots of minor problems that should be fixed. Here are some. Closing this ticket wouldn't require fixing them all, but if not, then explanations should be provided here and followup tickets (if needed) opened.

  • Awkward phrasing:
        Note that all the methods are numeric rather than analytic, for unusual
        regions or insufficient collocation points may give very inaccurate
        results.
    
    This is hard to follow, though I see what it says.
  • There is also a 'reimann' several times, and a 'correspondance'.
  • This also seems to have a transposition in how it's put in. Presumably the redefinition of I is supposed to demonstrate it works with lots of different complex types. I suggest the following.
            Can work for different types of complex numbers::
    
                sage: m = Riemann_Map([lambda t: e^(I*t) - 0.5*e^(-I*t)], [lambda t: I*e^(I*t) + 0.5*I*e^(-I*t)], 0)  # long time (4 sec)
                sage: m.riemann_map(0.25 + sqrt(-0.5))  # long time
                (0.137514137885...+0.876696023004...j)
    +            sage: I = CDF.gen()  # long time
                sage: m.riemann_map(1.3*I)  # long time
                (-1.561029396...+0.989694535737...j)
    -            sage: I = CDF.gen()  # long time
                sage: m.riemann_map(0.4)  # long time
    
  • Add any information at all about theoretical error bounds, if known. Since not everyone will be able to just look up that paper referenced, it would be helpful to have at least order of magnitude ideas (e.g., if N=2000 on a map from the unit circle to itself, we expect errors no greater than epsilon=blah).
  • From #8867: "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."

Change History (12)

comment:1 Changed 9 years ago by kcrisman

  • Description modified (diff)

comment:2 Changed 9 years ago by kcrisman

  • Description modified (diff)

comment:3 Changed 9 years ago by kcrisman

  • Description modified (diff)

comment:4 Changed 9 years ago by evanandel

Regarding the documentation, thanks for pointing that out. I'll get on it. Since I'm working on this as a senior project, I appreciate any documentation/setup feedback since I can stick that under the category of "usability testing".

I've separated the analytic tests under #10857. I'll get to work on that soon.

With regards to the plotting, I plan to do some reworking of that whole area. I'm going to add options for parallelization of the grid computation. I also intend to make the display function modular so that different "color schemes" can be used. For example I want the interested user to be able to specify the bright/dark ranges. I also want to add an error plot that shows estimates of the numerical error.

Now the reason I made my "clone" of complex_plot in the first place was to minimize the function call overhead by keeping it all in-house where I can take advantage of the cdef. However some of the work I want to do could be pretty general, thus it might be worth it to put it somewhere in the plot package.

Thoughts?

comment:5 follow-up: Changed 9 years ago by kcrisman

  • Description modified (diff)

Description updated.

I'm not the Cython expert. But if you cc: Jason and Robert Bradshaw, you should get some good help :) I don't see why one couldn't cimport the complex plot, after all. Maybe not? Modularity is good, I don't think people will be plotting so very many of these as to worry about the millisecond it takes to import.

comment:6 in reply to: ↑ 5 Changed 9 years ago by evanandel

Correction, analytic tests are under #10957

comment:7 Changed 9 years ago by kcrisman

  • Description modified (diff)

#11028 is taking care of the modularity and ColorPlot issues, I think.

comment:8 Changed 8 years ago by kcrisman

  • Reviewers set to Karl-Dieter Crisman
  • Status changed from new to needs_review

Okay, I've checked everything, and this is superseded by #11273 and #11028. Please close this ticket.

comment:9 Changed 8 years ago by kcrisman

  • Status changed from needs_review to positive_review

comment:10 Changed 8 years ago by kcrisman

  • Milestone set to sage-4.7.1

comment:11 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-4.7.1 to sage-duplicate/invalid/wontfix

comment:12 Changed 8 years ago by jdemeyer

  • Resolution set to duplicate
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.