Opened 8 years ago

Closed 6 years ago

#11273 closed enhancement (fixed)

Riemann Enhancements: Docs, Exterior, Multiple Spiderweb, Error Testing

Reported by: evanandel Owned by: burcin
Priority: major Milestone: sage-5.13
Component: calculus Keywords: riemann map
Cc: kcrisman, jason, fbissey, drkirby Merged in: sage-5.13.beta0
Authors: Ethan Van Andel Reviewers: Burcin Erocal, Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by kcrisman)

This is a major upgrade of the riemann module. Features are:

Fixes and expansions to the docs, covering the issues in #10945.

I've added some test methods that compute the explicit map of an ellipse to a high degree of accuracy. The doctests for these methods contain tests that show that the numerical Riemann_Map values remain accurate. The precision of the normal doctests has been reduced accordingly, so hopefully they will no longer fail for different architectures/numpy versions. This covers #10957

The exterior Riemann map can now be computed, that is a conformal mapping of the exterior of a region to the exterior of the unit circle. The docs specify how this may be done.

I've added support for computing the spiderweb plot for multiply connected domains. This uses a different method than the simply connected spiderweb and is takes about the same time as a color plot of the same resolution. (It's worth noting that I don't think this has ever been done before.)

I've also fiddled around under the hood, cleaning up some code, moving things around for more efficiency and trying to add a few more helpful inline comments.


Apply trac_11273-rebase-part1.patch, trac_11273-rebase-part2.patch, trac_11273-rebase-part3.patch, trac_11273-reviewer.patch, trac_11273-fix.patch, and trac_11273_multi_web_fix-review.patch.

Attachments (11)

trac-11273-riemann-upgrade-part1.patch (46.8 KB) - added by evanandel 8 years ago.
trac-11273-riemann-upgrade-part2.patch (2.3 KB) - added by evanandel 8 years ago.
trac-11273-riemann_upgrade_part3.patch (22.1 KB) - added by evanandel 8 years ago.
Formatting Fixes
trac_11273-rebase-part2.patch (3.5 KB) - added by kcrisman 7 years ago.
Rebased to 5.1.beta0
trac_11273-rebase-part3.2.patch (22.1 KB) - added by kcrisman 7 years ago.
Rebased to 5.1.beta0
trac_11273-fix.patch (9.3 KB) - added by evanandel 7 years ago.
trac_11273-rebase-part1.patch (47.1 KB) - added by kcrisman 6 years ago.
Rebased to 5.1.beta0
trac_11273-rebase-part3.patch (21.8 KB) - added by kcrisman 6 years ago.
Rebased to 5.1.beta0
trac_11273-reviewer.patch (31.2 KB) - added by kcrisman 6 years ago.
trac_11273_multi_web_fix.patch (6.2 KB) - added by evanandel 6 years ago.
A fix for exterior fuzz in some multiply connected spiderwebs.
trac_11273_multi_web_fix-review.patch (6.2 KB) - added by kcrisman 6 years ago.

Download all attachments as: .zip

Change History (46)

Changed 8 years ago by evanandel

Changed 8 years ago by evanandel

comment:1 follow-up: Changed 8 years ago by evanandel

  • Status changed from new to needs_review

Because of the complexity of these changes, I decided I didn't want to deal with separating each batch out into its own patch. Hopefully that is acceptable.

As a result, this ticket should replace #10945 (some of the other issues there have already been addressed, this just hits the documentation) and #10957. Is there a good way to go about doing that?

Also, as a side note, let me know if there are any places where you would like to see more documentation or tests.

comment:2 in reply to: ↑ 1 Changed 8 years ago by burcin

  • Dependencies changed from 8867, 10792, 10821, 11028 to #8867, #10792, #10821, #11028

Replying to evanandel:

As a result, this ticket should replace #10945 (some of the other issues there have already been addressed, this just hits the documentation) and #10957. Is there a good way to go about doing that?

We should close those tickets as duplicates of this one.

The patch looks good to me. There are a few minor formatting issues:

  • lines should be <= 80 characters. The patch contains some lines with very aggressive wrapping, such as
              q_vector = 1 / ( 
    	                self.pre_q_vector - pt1) 
    
    and some with almost none.
  • There should be an empty line in the documentation after :: to start a code block.
  • I would appreciate it if there were more comments in the code to explain what it's doing, but this is not a requirement.
  • attachment:trac-11273-riemann-upgrade-part2.patch cuts down the precision required in the doctests to 5 digits in some places. Is this code really so unstable? Would it help to use other functions for numeric approximation, other than those defined in python's math module?

I am leaving this as needs_review since I don't consider this a full review. Somebody more familiar with this code should comment on the changes and check if this ticket does indeed replace #10945 and #10957.

comment:3 Changed 8 years ago by kcrisman

  • Reviewers set to Burcin Erocal, Karl-Dieter Crisman
  • Status changed from needs_review to needs_work

A few other comments.

First, ones to make sure it replaces #10945.

  • 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
    
    • 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."

Then a few other comments.

  • Incorrect indentation in the last couple functions (in addition to the empty lines Burcin mentioned).
  • Some math things could be put in single backticks, like 2*pi.
  • Commit messages should have the Trac ticket number in them, like Trac 11273, major additions"
  • Commit message should have a short first line, then the rest of the information.

Hope to later do a better review of the content, which I think I will be able to do. This does need some work due to all these formatting etc. issues, though.

Changed 8 years ago by evanandel

Formatting Fixes

comment:4 Changed 8 years ago by evanandel

  • Status changed from needs_work to needs_review

Alright I've added a patch that should fix all the formatting fixes mentioned.

Regarding code comments, given the complexity of the algorithm, it seemed more suitable to me to link to the appropriate mathematical papers than to attempt to fully document the algorithm in the code.

Regarding test precision, no the code is not that unstable. It seems stable to about 10 decimal places or so (although not for nearly 0 values). However, being an numerical algorithm, it is only accurate to roughly 5 decimal places for the number of collocation points in the tests. Therefore it seemed appropriate to try and match the tests to that precision so that if future tweaks change the results--maybe even making them more precise--that won't cause the tests to fail unnecessarily.

comment:5 Changed 8 years ago by kcrisman

Here are some more precise remaining comments.

  • As text files, you should make sure to include the Trac number in the commit messages of the first two patches. I recommend just opening them as text, adding that, and then uploading them again ("replace existing file of the same name" or whatever). No need to rebase or something like that, that would be silly when this is so easy to fix.
  • The indention of Simple test:: and Testing the accuracy of Riemann_Map:: is still not correct. Those should be at the same level as the TESTS: block, I think.
  • In order to make sure we actually we right in closing #8867 and #10945, there has to be some mention of lambda functions. You said so yourself :)
  • I still think that the change in 3 needs to be made, or that test doesn't make sense. If I'm wrong, just clarify, no big deal.

As to review thus far, I have no problem with patch 2 as long as it passes tests, and patch 3 is clearly fine except for the issues mentioned above, which should be fixed.

So now I just need to make sure that the overhaul and new stuff in patch 1 is right :) Though I trust it is. I'm looking forward to this being in and at this level, even if I don't get to teach complex analysis for a while.

comment:6 Changed 8 years ago by evanandel

  • Status changed from needs_review to needs_info

I made the necessary changes. However, now when I try to apply even the original patches the process fails giving me

            
            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: isinstance(Riemann_Map([f], [fprime], 0)._repr_(), str)  # long time
            True
        """
        return "A Riemann mapping of a figure to the unit circle."

    cdef _generate_theta_array(self):
        """

I have absolutely no idea what is causing this. I double checked the first hunk and as far as I can tell what is there exactly matches what the patch expects to find. What is the best way to figure out exactly why these hunks are failing to apply?

comment:7 Changed 8 years ago by jason

Can you tell us exactly what you are doing to apply the patches and exactly what it is saying?

comment:8 Changed 8 years ago by evanandel

Apologies. I posted the wrong bit of text above.

I am starting from a clean 4.7.2. First I apply the patch for ticket #11028.

sage: hg_sage.apply("trac-11028-modular-complex-plot.2.3.patch")
cd "/home/ethan/sage/devel/sage" && hg import   "/home/ethan/sage/trac-11028-modular-complex-plot.2.3.patch"
applying /home/ethan/sage/trac-11028-modular-complex-plot.2.3.patch

The patch applies correctly, and the tests run properly.

I then try to apply the first patch from this ticket and get the following result:

sage: hg_sage.apply("trac-11273-riemann-upgrade-part1.patch")   
cd "/home/ethan/sage/devel/sage" && hg import   "/home/ethan/sage/trac-11273-riemann-upgrade-part1.patch"
applying /home/ethan/sage/trac-11273-riemann-upgrade-part1.patch
patching file sage/calculus/riemann.pyx
Hunk #9 FAILED at 264
Hunk #28 FAILED at 1012
Hunk #29 FAILED at 1051
3 out of 30 hunks FAILED -- saving rejects to file sage/calculus/riemann.pyx.rej
abort: patch failed to apply

I could not figure out from the .rej file why the patch is failing. There seem to be no incompatibilities in the significant hunks.

comment:9 Changed 7 years ago by kcrisman

  • Dependencies changed from #8867, #10792, #10821, #11028 to #11028

Updating so as not to have additional dependencies for patchbot.

#11028 has positive review, so now I'll start up on this one. First I'll try to rebase if needed.

Changed 7 years ago by kcrisman

Rebased to 5.1.beta0

Changed 7 years ago by kcrisman

Rebased to 5.1.beta0

comment:10 Changed 7 years ago by kcrisman

  • Description modified (diff)

Apply trac_11273-rebase-part1.patch, trac_11273-rebase-part2.patch, and trac_11273-rebase-part3.patch.


Current status is having rebased and fixed a few minor things along the way.

Need to:

  • Actually review the substantive changes, though these look good at first
  • Remove a lot of whitespace

comment:11 follow-up: Changed 7 years ago by kcrisman

Okay, Ethan, here come the questions! Notice I'm still in the middle of reviewing, so I intend to fix a lot of little things yet in followup patches.

  • You say
    The following inputs must all be passed in as named parameters:
    
    but they don't, since they already are named, not **kwds (I even tried). Are you thinking of putting in extra arguments later or something? You say something similar with the int argument in get_theta_points.
  • There is a problem with the non-simply-connected plots. Not a big one! But for some reason they don't add properly - maybe you put them in the wrong zorder. Check out these two.
            sage: f(t) = e^(I*t)
            sage: fprime(t) = I*e^(I*t)
            sage: hf(t) = 0.5*e^(-I*t)
            sage: hfprime(t) = 0.5*-I*e^(-I*t)
            sage: m = Riemann_Map([f, hf], [fprime, hfprime], 0.5 + 0.5*I)
            sage: m.plot_colored() + m.plot_spiderweb()  # long time
            sage: m.plot_spiderweb()+m.plot_colored()
    
    I have a feeling this is because of the direct use of a ComplexPlot in the "more difficult multiply connected" branch of the plot_spiderweb, when you already are using a ComplexPlot in the plot_colored, but I'm not sure exactly how this happens, and would rather not track it down.

comment:12 Changed 7 years ago by kcrisman

More. This first one makes it "needs work". Best to just add another patch after my reviewer patch, which is coming after I finish reviewing.

  • I see no doctest for the new exterior keyword. This is important new functionality, so we need it. I would add it myself, but
    sage: f(t) = e^(I*t)
    sage: fprime(t) = I*e^(I*t)
    sage: m = Riemann_Map([f], [fprime], 0)  # long time (4 sec)
    sage: m.plo
    m.plot_boundaries  m.plot_colored     m.plot_spiderweb   
    sage: m.plot_spiderweb()+m.plot_colored()  # looks good
    sage: m = Riemann_Map([f], [fprime], 0, exterior=True)  # long time (4 sec)
    sage: m.plot_spiderweb()+m.plot_colored()  # looks weird
    
    has a small colored square around a black interior regions, but long long spiderweb, and I'm not sure this is intentional. The same thing happens for other ones made exterior. Also, testing the ValueError raised would be good.
  • Why the difference in the initialization between self.exterior and exterior? Sometimes you use one, sometimes the other, and it's confusing.
  • In general a few of the things for exterior are a little confusing. For instance, pre_q_vector seems to be generated in the same way for both exterior=True and False, whereas nearly everything else is inverted through the origin.
  • I don't know if this needs to be done, but there are places where some checking could be done - like y_points = int((ymax-ymin)/ ystep) seems to be asking for trouble if you ended up with zero for some reason, though in practice one would probably have to try hard to do that.
  • You are a little inconsistent in use of your 'global'-ish names like COMPLEX - sometimes you use it, sometimes you don't.
  • This was a little hard to understand.
    #b/c the function is analytic, we know it's abs(derivative) is equal in all directions 
    
    It's not clear exactly what this means, as the derivative of a complex-valued function is a complex number. And partial derivatives don't have to be the same in all directions. I feel like I almost get it, but a little clarification of what you meant by this would be helpful.
  • A few nice (pictorial) examples of why the multiply connected spider plots code works would be good; it seems right from the ones I tried, but seeing some examples would be even better.
  • This is probably too much to ask... would it be possible to have the last three functions (for testing the ellipse example) take epsilon as a parameter instead of just .3? This would help me check whether this is really doing the right thing, though I didn't really have any doubts, given the great pictures!

Maybe you should just send me the Involve article off-list, or have Mike B. send it to me (I don't have access to it). Then I'd really feel confident signing off on this. That said, it really looks good, with the exception of a few mathematical things I want more detail on.

comment:13 Changed 7 years ago by kcrisman

  • Status changed from needs_info to needs_review
  • Work issues set to see comments

One final thing, which John Palmieri may have to help on. There are warnings when building the documentation that I can't figure out.

/Users/.../sage-5.1.beta0/devel/sage/doc/en/reference/sage/calculus/riemann.rst:9: WARNING: error while formatting signature for sage.calculus.riemann.complex_to_rgb: Could not parse cython argspec
/Users/.../sage-5.1.beta0/devel/sage/doc/en/reference/sage/calculus/riemann.rst:9: WARNING: error while formatting signature for sage.calculus.riemann.complex_to_spiderweb: Could not parse cython argspec
/Users/.../sage-5.1.beta0/devel/sage/doc/en/reference/sage/calculus/riemann.rst:9: WARNING: error while formatting signature for sage.calculus.riemann.get_derivatives: Could not parse cython argspec

Though looking at some other tickets, I'm wondering whether this is just something our formatting thing isn't ready for...

comment:14 Changed 7 years ago by kcrisman

  • Status changed from needs_review to needs_work

comment:16 in reply to: ↑ 11 ; follow-up: Changed 7 years ago by evanandel

Thanks for the review. I've tried to address the issues you raise.

Replying to kcrisman:

Okay, Ethan, here come the questions! Notice I'm still in the middle of reviewing, so I intend to fix a lot of little things yet in followup patches.

  • You say
    The following inputs must all be passed in as named parameters:
    
    but they don't, since they already are named, not **kwds (I even tried). Are you thinking of putting in extra arguments later or something? You say something similar with the int argument in get_theta_points.

Updated these descriptions to say "may be passed in". Technically the name isn't required, but it seems helpful to advise it for users less familiar with sage arguments.

  • There is a problem with the non-simply-connected plots. Not a big one! But for some reason they don't add properly - maybe you put them in the wrong zorder. Check out these two.
            sage: f(t) = e^(I*t)
            sage: fprime(t) = I*e^(I*t)
            sage: hf(t) = 0.5*e^(-I*t)
            sage: hfprime(t) = 0.5*-I*e^(-I*t)
            sage: m = Riemann_Map([f, hf], [fprime, hfprime], 0.5 + 0.5*I)
            sage: m.plot_colored() + m.plot_spiderweb()  # long time
            sage: m.plot_spiderweb()+m.plot_colored()
    
    I have a feeling this is because of the direct use of a ComplexPlot in the "more difficult multiply connected" branch of the plot_spiderweb, when you already are using a ComplexPlot in the plot_colored, but I'm not sure exactly how this happens, and would rather not track it down.

Good catch. The plots don't add because they are both image type plots, rather than the line plot in the simply connected spiderweb case. To get a color+spiderweb plot you need to use the "withcolor=True" flag. I've updated the docs and doctests to accurately reflect this.

Replying to kcrisman:

More. This first one makes it "needs work". Best to just add another patch after my reviewer patch, which is coming after I finish reviewing.

  • I see no doctest for the new exterior keyword. This is important new functionality, so we need it. I would add it myself, but
    sage: f(t) = e^(I*t)
    sage: fprime(t) = I*e^(I*t)
    sage: m = Riemann_Map([f], [fprime], 0)  # long time (4 sec)
    sage: m.plo
    m.plot_boundaries  m.plot_colored     m.plot_spiderweb   
    sage: m.plot_spiderweb()+m.plot_colored()  # looks good
    sage: m = Riemann_Map([f], [fprime], 0, exterior=True)  # long time (4 sec)
    sage: m.plot_spiderweb()+m.plot_colored()  # looks weird
    
    has a small colored square around a black interior regions, but long long spiderweb, and I'm not sure this is intentional. The same thing happens for other ones made exterior. Also, testing the ValueError raised would be good.

Added the appropriate doctests. Spiderweb plots don't work for exterior maps and are now checked for appropriately.

  • Why the difference in the initialization between self.exterior and exterior? Sometimes you use one, sometimes the other, and it's confusing.

Fixed.

  • In general a few of the things for exterior are a little confusing. For instance, pre_q_vect.r seems to be generated in the same way for both exterior=True and False, whereas nearly everything else is inverted through the origin.

Note the pre_q_vector is basically just a portion of self.cps which is adjusted for exterior,

  • I don't know if this needs to be done, but there are places where some checking could be done - like y_points = int((ymax-ymin)/ ystep) seems to be asking for trouble if you ended up with zero for some reason, though in practice one would probably have to try hard to do that.

I'm going to pass on this for now. I've tried to catch the less obvious gotchas, but testing all combinations of bad parameters seems like overkill.

  • You are a little inconsistent in use of your 'global'-ish names like COMPLEX - sometimes you use it, sometimes you don't.
  • This was a little hard to understand.
    #b/c the function is analytic, we know it's abs(derivative) is equal in all directions 
    
    It's not clear exactly what this means, as the derivative of a complex-valued function is a complex number. And partial derivatives don't have to be the same in all directions. I feel like I almost get it, but a little clarification of what you meant by this would be helpful.

Clarified. Let me know if it's still unclear.

  • A few nice (pictorial) examples of why the multiply connected spider plots code works would be good; it seems right from the ones I tried, but seeing some examples would be even better.

I'm not sure what you mean by examples showing how it works.

  • This is probably too much to ask... would it be possible to have the last three functions (for testing the ellipse example) take epsilon as a parameter instead of just .3? This would help me check whether this is really doing the right thing, though I didn't really have any doubts, given the great pictures!

Done.

Maybe you should just send me the Involve article off-list, or have Mike B. send it to me (I don't have access to it). Then I'd really feel confident signing off on this. That said, it really looks good, with the exception of a few mathematical things I want more detail on.

Done. Let me know if you didn't get the email.

Changed 7 years ago by evanandel

comment:17 Changed 7 years ago by evanandel

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

comment:18 in reply to: ↑ 16 Changed 7 years ago by kcrisman

Thanks for the review. I've tried to address the issues you raise.

Thanks!

Done. Let me know if you didn't get the email.

I did get it. Haven't had time to return to this yet - given that I need to read the article yet. But I am hopeful to review this in 2013 ;-)

comment:19 Changed 7 years ago by jdemeyer

  • Description modified (diff)
  • Work issues see comments deleted

comment:20 Changed 7 years ago by jdemeyer

For a particular build of ATLAS, this still gives

sage -t  "devel/sage/sage/calculus/riemann.pyx"
**********************************************************************
File "/home/buildbot/sage-5.6.rc1/devel/sage/sage/calculus/riemann.pyx", line 675:
    sage: m.inverse_riemann_map(0.5 + sqrt(-0.5))
Expected:
    (0.406880...+0.3614702...j)
Got:
    (0.26609535153324926-0.71596339253951613j)
**********************************************************************
File "/home/buildbot/sage-5.6.rc1/devel/sage/sage/calculus/riemann.pyx", line 677:
    sage: m.inverse_riemann_map(0.95)
Expected:
    (0.486319...-4.90019052...j)
Got:
    (-0.47636117038850095-0.31131054266912134j)
**********************************************************************
File "/home/buildbot/sage-5.6.rc1/devel/sage/sage/calculus/riemann.pyx", line 679:
    sage: m.inverse_riemann_map(0.25 - 0.3*I)
Expected:
    (0.1653244...-0.180936...j)
Got:
    (-0.22158720103166435+0.057314947441646828j)
**********************************************************************
File "/home/buildbot/sage-5.6.rc1/devel/sage/sage/calculus/riemann.pyx", line 682:
    sage: m.inverse_riemann_map(np.complex(-0.2, 0.5))
Expected:
    (-0.156280...+0.321819...j)
Got:
    (0.30335339499315012-0.10111072188371266j)
**********************************************************************
File "/home/buildbot/sage-5.6.rc1/devel/sage/sage/calculus/riemann.pyx", line 783:
    sage: print data[0][8,1]
Expected:
    (-0.0879...+0.9709...j)
Got:
    (-0.596032167594-0.809542574347j)
**********************************************************************
File "/home/buildbot/sage-5.6.rc1/devel/sage/sage/calculus/riemann.pyx", line 1086:
    sage: dr[8,8]
Expected:
    0.241...
Got:
    0.27207542829155734
**********************************************************************
File "/home/buildbot/sage-5.6.rc1/devel/sage/sage/calculus/riemann.pyx", line 1088:
    sage: dtheta[5,5]
Expected:
    5.907...
Got:
    5.664338752865179
**********************************************************************
File "/home/buildbot/sage-5.6.rc1/devel/sage/sage/calculus/riemann.pyx", line 1440:
    sage: abs(m.riemann_map(.5)-analytic_interior(.5, 20, .3)) < 10^-6
Expected:
    True
Got:
    False
**********************************************************************
File "/home/buildbot/sage-5.6.rc1/devel/sage/sage/calculus/riemann.pyx", line 411:
    sage: s(3*pi / 4)
Expected:
    0.0012158...
Got:
    0.0011753476894319776
**********************************************************************
File "/home/buildbot/sage-5.6.rc1/devel/sage/sage/calculus/riemann.pyx", line 484:
    sage: s(3*pi / 4)
Expected:
    1.627660...
Got:
    -2.2112992320625007
**********************************************************************
File "/home/buildbot/sage-5.6.rc1/devel/sage/sage/calculus/riemann.pyx", line 589:
    sage: m.riemann_map(0.25 + sqrt(-0.5))
Expected:
    (0.137514...+0.876696...j)
Got:
    (-0.72646985217696924-0.51090507937918783j)
**********************************************************************
File "/home/buildbot/sage-5.6.rc1/devel/sage/sage/calculus/riemann.pyx", line 591:
    sage: m.riemann_map(1.3*I)
Expected:
    (-1.56...e-05+0.989694...j)
Got:
    (-0.70519318141122977-0.76115177043780835j)
**********************************************************************
File "/home/buildbot/sage-5.6.rc1/devel/sage/sage/calculus/riemann.pyx", line 594:
    sage: m.riemann_map(0.4)
Expected:
    (0.73324...+3.2...e-06j)
Got:
    (-0.45716531868823951+0.52177153514479202j)
**********************************************************************
File "/home/buildbot/sage-5.6.rc1/devel/sage/sage/calculus/riemann.pyx", line 597:
    sage: m.riemann_map(np.complex(-3, 0.0001))
Expected:
    (1.405757...e-05+8.06...e-10j)
Got:
    (-0.014156297786253731+0.012030907696686667j)
**********************************************************************

These failures aren't new, but it would be good to fix them.

comment:21 Changed 7 years ago by fbissey

Out of curiosity. You say "a particular build of ATLAS" and "this is not new". So this is reproducible on a particular machine or it crops up every so often?

comment:22 Changed 7 years ago by jdemeyer

See sage-devel.

It crops up every so often on sage.math and is "fixed" by rebuilding ATLAS. But since the only failures are in riemann.pyx, I find it unlikely that ATLAS is really the culprit.

comment:23 follow-up: Changed 6 years ago by kcrisman

Ah, patchbot...

Apply trac_11273-rebase-part1.patch, trac_11273-rebase-part2.patch, trac_11273-rebase-part3.patch, trac_11273-reviewer.patch, and trac_11273-fix.patch.

Just for my reference, here are the remaining things I noticed and/or I have to do before giving positive review. Probably some of these can go to other tickets at this point. I'm pretty happy with this overall.

  • Mysterious
    Converts a list of complex numbers xderivs = get_derivatives(z_values, xstep, ystep)[0]to the plottable 
    
  • Typo equallyspaced
  • Still haven't addressed "Presumably the redefinition of I is ..." (new ticket?)
  • Or "It now properly avoids failing with lambda functions" (new ticket?)
  • Need to read papers (will do now)
  • Possibly some formatting of examples
  • And test all examples again by hand to make sure something didn't slip through!

comment:24 Changed 6 years ago by kcrisman

Aargh. Needs rebase due to #11334.

Changed 6 years ago by kcrisman

Rebased to 5.1.beta0

Changed 6 years ago by kcrisman

Rebased to 5.1.beta0

comment:25 Changed 6 years ago by kcrisman

Okay, it was some work but doable.

Patchbot, apply trac_11273-rebase-part1.patch, trac_11273-rebase-part2.patch, trac_11273-rebase-part3.patch, trac_11273-reviewer.patch, and trac_11273-fix.patch.

I'll get to the rest of this tonight, but hopefully soon all ready!

comment:26 in reply to: ↑ 23 Changed 6 years ago by kcrisman

Ah, patchbot...

Thank you, patchbot! Green means go.

  • Mysterious
    Converts a list of complex numbers xderivs = get_derivatives(z_values, xstep, ystep)[0]to the plottable 
    
  • Typo equallyspaced

Both long ago fixed, not sure how I missed this.

  • Still haven't addressed "Presumably the redefinition of I is ..."

I will probably make this a friendly revision to the reviewer patch.

  • Or "It now properly avoids failing with lambda functions" (new ticket?)

Well, there's at least one example with a lambda function, so let's let that pass...

comment:27 Changed 6 years ago by kcrisman

In general, I'm happy here. I still find this very interesting and useful.

Just one thing; I took an example from the paper and did some stuff.

ps = polygon_spline([(-4,-2),(4,-2),(4,2),(-4,2)])
z1 = lambda t: ps.value(t); z1p = lambda t: ps.derivative(t)
z2(t) = -2+exp(-I*t); z2p(t) = -I*exp(-I*t)
z3(t) = 2+exp(-I*t); z3p(t) = -I*exp(-I*t)
m = Riemann_Map([z1,z2,z3],[z1p,z2p,z3p],0,ncorners=4)
p = m.plot_spiderweb(withcolor=True,plot_points=500)
show(p,axes=false)

This one has some of the issues mentioned in the thesis in terms of some weirdness of the spiderweb (in particular, that all the correct stuff is there, but some additional webs as well). I've sent the author an email - perhaps we will want to ask people to not use it with domains too far from being simply connected.

Changed 6 years ago by kcrisman

comment:28 Changed 6 years ago by kcrisman

  • Dependencies #11028 deleted
  • Status changed from needs_review to needs_info

Lest the patchbot try too hard... apply trac_11273-rebase-part1.patch, trac_11273-rebase-part2.patch, trac_11273-rebase-part3.patch, trac_11273-reviewer.patch, and trac_11273-fix.patch.

comment:29 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:30 Changed 6 years ago by kcrisman

Update: From personal correspondence, looks like the check for those lines not being drawn was somehow removed in the long sequence of patches here. Likely to have a patch relatively soon.

Changed 6 years ago by evanandel

A fix for exterior fuzz in some multiply connected spiderwebs.

comment:31 Changed 6 years ago by evanandel

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

It seems that for some domains, particularly those with corners, the numerical integration outside the corners is bad enough to cause "bleed" so that the value outside the domain is farther from zero than desirable. This meant that those portions were escaping the normal check to not draw external lines.

I've made the cutoff a parameter (min_mag) so that the user can make it higher if they're observing lines outside the domain. The only trade-off is that the lines will stop slightly farther from the "centers" of the domain. There is a doctest illustrating the increased cutoff.

At the request of the reviewer in correspondence, I've also added a fun example to the top level doctests.

comment:32 Changed 6 years ago by kcrisman

This will need a little TLC for a couple minor formatting things, but I think this fix makes sense. I'll check it out, thanks!

comment:33 Changed 6 years ago by kcrisman

Okay, this looks great. Thank you so much for finishing these last tweaks. I'll upload the formatting fix version of your latest patch and we'll be good to go!

Changed 6 years ago by kcrisman

comment:34 Changed 6 years ago by kcrisman

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

Positive review!

Patchbot, apply trac_11273-rebase-part1.patch, trac_11273-rebase-part2.patch, trac_11273-rebase-part3.patch, trac_11273-reviewer.patch, trac_11273-fix.patch, trac_11273_multi_web_fix-review.patch

comment:35 Changed 6 years ago by jdemeyer

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