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 )
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)
Change History (46)
Changed 8 years ago by
Changed 8 years ago by
comment:1 follow-up: ↓ 2 Changed 8 years ago by
- Status changed from new to needs_review
comment:2 in reply to: ↑ 1 Changed 8 years ago by
- 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
- 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
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.
comment:4 Changed 8 years ago by
- 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
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 ofRiemann_Map::
is still not correct. Those should be at the same level as theTESTS:
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
- 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
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
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
- 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.
comment:10 Changed 7 years ago by
- 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: ↓ 16 Changed 7 years ago by
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 theint
argument inget_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 aComplexPlot
in the "more difficult multiply connected" branch of theplot_spiderweb
, when you already are using aComplexPlot
in theplot_colored
, but I'm not sure exactly how this happens, and would rather not track it down.
comment:12 Changed 7 years ago by
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, butsage: 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 theValueError
raised would be good. - Why the difference in the initialization between
self.exterior
andexterior
? 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 bothexterior=True
andFalse
, 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
- 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
- Status changed from needs_review to needs_work
comment:15 Changed 7 years ago by
- Description modified (diff)
comment:16 in reply to: ↑ 11 ; follow-up: ↓ 18 Changed 7 years ago by
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 theint
argument inget_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 aComplexPlot
in the "more difficult multiply connected" branch of theplot_spiderweb
, when you already are using aComplexPlot
in theplot_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, butsage: 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 weirdhas 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 theValueError
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
andexterior
? 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 bothexterior=True
andFalse
, 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 directionsIt'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
comment:17 Changed 7 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
comment:18 in reply to: ↑ 16 Changed 7 years ago by
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
- Description modified (diff)
- Work issues see comments deleted
comment:20 Changed 7 years ago by
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
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
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: ↓ 26 Changed 6 years ago by
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
Aargh. Needs rebase due to #11334.
comment:25 Changed 6 years ago by
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
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
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
comment:28 Changed 6 years ago by
- 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
- Milestone changed from sage-5.11 to sage-5.12
comment:30 Changed 6 years ago by
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.
comment:31 Changed 6 years ago by
- 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
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
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
comment:34 Changed 6 years ago by
- 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
- Merged in set to sage-5.13.beta0
- Resolution set to fixed
- Status changed from positive_review to closed
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.