search

sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.37k stars 466 forks source link

More Modular ComplexPlot #11028

Closed 6b548601-af37-463d-9179-81ac1de95057 closed 12 years ago

6b548601-af37-463d-9179-81ac1de95057 commented 13 years ago

I modified ComplexPlot to be more flexible and modular. The __init__ method now takes rgb_data instead of z_values as input. This permits users to decide how they want to represent their complex data. Adjustments have been made to both RiemannMap.plot_colored and complex_plot.

I also removed the now-redundant ColorPlot from riemann.pyx.

Apply attachment: trac-11028-modular-complex-plot.2.3.patch and attachment: trac_11028-reviewer.patch.

CC: @kcrisman @robertwb

Component: graphics

Keywords: complex plot riemann map

Author: Ethan Van Andel

Reviewer: Karl-Dieter Crisman

Merged: sage-5.1.beta2

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

kcrisman commented 13 years ago
comment:2

It seems like this has a slight redundancy with the (positively reviewed) #10821. In fact, it looks like whatever this is based on has some, but not all, of the changes there, and the other changes are in this patch. ?? Anyway, the specific order and so forth of dependencies should be completely clarified.

The patch is also apparently a double patch. Read it and you'll see what I mean. Which situation with regard to the not being able to test cdef'd functions is current? (Sometimes we create a def'd test_my_cdef_function for these cases.)

Will we need to introduce a deprecation period for the change to initializing with complex_to_rgb(z_values), since the keyword has changed?

Also, what is the situation with the complex_to_rgb functions? By that I mean to ask how many of them there are, and why there is a separate one in riemann.pyx.

kcrisman commented 13 years ago
comment:3

With respect to #10945, please also make sure that the (better) docs you had for ColorPlot are transferred to ComplexPlot; we definitely wouldn't want to lose them!

To be more precise about complex_to_rgb, there should probably only be one of them, it should probably be in the complex plot place as importing it once (or cimporting?) shouldn't cause any noticeable slowdown (though you may want to check that), and there is no reason not to use the efficiencies you have introduced in that final version :)

6b548601-af37-463d-9179-81ac1de95057 commented 13 years ago
comment:4

You're absolutely right about the patch issues. I'm not sure how it got doubled... The top half, with the actual test cases is more current, those methods should be testing properly. Also, the redundant changes seem to be a slightly older version of my error handling, I'm not sure why they're in this one. I'm going to try and fix up the patch manually.

With regards to complex_to_rgb, there are two versions because Riemann assigns colors slightly differently than complex_plot. Thus the methods really are intentionally somewhat different. Also, I don't think we need a deprecation period for the complex_to_rgb or ComplexPlot calls. That seems to a fairly specific internal method for complex_plot. I have a hard time imagining someone else using that stuff without going through complex_plot() which has been changed appropriately.

That said, I don't know what the official policy is on such matters. Robert Bradshaw could probably give a better answer as to whether the changes will break anything.

Finally, I'm not sure what you mean by the better docs for ColorPlot. What specifically are you referring to?

kcrisman commented 13 years ago

Reviewer: Karl-Dieter Crisman

kcrisman commented 13 years ago

Changed author from evanandel to Ethan Van Andel

kcrisman commented 13 years ago
comment:5

That said, I don't know what the official policy is on such matters. Robert Bradshaw could probably give a better answer as to whether the changes will break anything.

It really is on a case-by-case basis with 'internal' things that users aren't supposed to see. Might be worth asking Robert and Jason, I agree.

Finally, I'm not sure what you mean by the better docs for ColorPlot. What specifically are you referring to?

Hmm, I just seem to remember that you had added some detail for ColorPlot that wasn't in the ComplexPlot documentation. If they are identical, then I must have been wrong. If not, might as well add it, since it helps! Or helped me.

6b548601-af37-463d-9179-81ac1de95057 commented 13 years ago
comment:6

OK, I uploaded a new patch that takes care of the double patch and redundancy and also adds a tiny bit to the ComplexPlot documentation (that will probably never be seen) let me know if there are any other stupid mistakes I made.

Once we get the go ahead from Robert Bradshaw this can be changed to needs_review. I'll email him if he doesn't see this in a couple of days.

kcrisman commented 13 years ago
comment:7

Putting as 'needs review' since this probably isn't a big deal, you are right. Still, in general internal functions should start with an underscore or something. And who knows?

This could be used in the future... see for instance this stackoverflow question, where they say "I've never come across any ready-made solution to your problem." Here it is!

kcrisman commented 13 years ago

Description changed:

kcrisman commented 13 years ago
comment:9

Apply only attachment: trac-11028-modular-complex-plot.2.2.patch.

kcrisman commented 13 years ago

Description changed:

--- 
+++ 
@@ -2,3 +2,5 @@

 I also removed the now-redundant ColorPlot from riemann.pyx.

+---
+Apply only [attachment: trac-11028-modular-complex-plot.2.2.patch](https://github.com/sagemath/sage/files/ticket11028/trac-11028-modular-complex-plot.2.2.patch.gz).
kcrisman commented 13 years ago
comment:10

A few thoughts:

So 'needs info', at least. These are all meta-issues in some way. The details seem fine! (Haven't actually applied or run tests, as I'm currently doing so with a different big job - but I have few doubts on that score.)

By the way, give my best to Mike Bolt. He told me he worked with a student in this paper in Involve, and I knew you were at Calvin, but somehow I didn't put it all together!

6b548601-af37-463d-9179-81ac1de95057 commented 13 years ago
comment:11

Those are my thoughts on the issues raised. Does that clear things up?

kcrisman commented 13 years ago
comment:12

Replying to @sagetrac-evanandel:

  • Those redefinitions could be omitted, but I generally think that it makes it more readable for each example section to define its functions unless it's very time or space consuming. If that violates a sage convention though, it can certainly be changed.

Ok.

  • It may make more sense to put ColorPlot as the main method. If that is the case, however, then it seems like color plot should be given its own module (it certainly doesn't belong in complex_plot) and then perhaps complex_plot() should move elsewhere since it's not apparent that it needs a whole module for just a couple functions. If that is the case though, that seems like a refactoring decision that is above my (hypothetical) pay grade. If it turns out to be complicated, it might be better just to let this patch go through and deal with that question separately.

Well, that's true.

  • I think the only difference between the two complex_to_rgb functions is that each one uses a different mag_to_lightness function. I couldn't figure out a better way to avoid code re-use since it's reasonable to expect other methods that access ColorPlot to define complex_to_rgb methods that differ in more than just mag_to_lightness, or for the existing methods to be modified later without wishing to affect the other.

Ok, I guess my usual plan is to push refactoring to later, too :)

Those are my thoughts on the issues raised. Does that clear things up?

Yes.

So I'll try to review this, and then finish #11273.

kcrisman commented 12 years ago
comment:13

At SD35.5, we found out that tests don't pass for some reason dealing with complex_to_rgb and the very first example formats weirdly now. Maybe that's my fault from when I rebased this?

6b548601-af37-463d-9179-81ac1de95057 commented 12 years ago

Attachment: trac-11028-modular-complex-plot.2.3.patch.gz

Fixed the test failures

6b548601-af37-463d-9179-81ac1de95057 commented 12 years ago

Description changed:

--- 
+++ 
@@ -3,4 +3,4 @@
 I also removed the now-redundant ColorPlot from riemann.pyx.

 ---
-Apply only [attachment: trac-11028-modular-complex-plot.2.2.patch](https://github.com/sagemath/sage/files/ticket11028/trac-11028-modular-complex-plot.2.2.patch.gz).
+Apply only [attachment: trac-11028-modular-complex-plot.2.3.patch](https://github.com/sagemath/sage-prod/files/10652484/trac-11028-modular-complex-plot.2.3.patch.gz).
6b548601-af37-463d-9179-81ac1de95057 commented 12 years ago
comment:14

It turns out those test failures were because riemann's complex_to_rgb is cdefed and therefore not importable for outside modules. I fixed that changing it to a cpdef.

I'm not sure if I just didn't catch those failures before, or if something changed to make it a problem. Regardless, it should work now.

kcrisman commented 12 years ago
comment:15

I've changed a few documentation things that weren't quite right, and added a doctest and explanation since you now only use Numpy arrays in this particular complex_to_rgb - which is fine, but then we need to tell people.

Apply trac-11028-modular-complex-plot.2.3.patch and trac_11028-reviewer.patch

kcrisman commented 12 years ago

Description changed:

--- 
+++ 
@@ -3,4 +3,4 @@
 I also removed the now-redundant ColorPlot from riemann.pyx.

 ---
-Apply only [attachment: trac-11028-modular-complex-plot.2.3.patch](https://github.com/sagemath/sage-prod/files/10652484/trac-11028-modular-complex-plot.2.3.patch.gz).
+Apply [attachment: trac-11028-modular-complex-plot.2.3.patch](https://github.com/sagemath/sage-prod/files/10652484/trac-11028-modular-complex-plot.2.3.patch.gz) and [attachment: trac_11028-reviewer.patch](https://github.com/sagemath/sage-prod/files/10652485/trac_11028-reviewer.patch.gz).
kcrisman commented 12 years ago

Attachment: trac_11028-reviewer.patch.gz

kcrisman commented 12 years ago
comment:16

Sorry for the very long wait, Ethan - nice stuff came out of this. Positive review, with the reviewer patch.

Now I can finally do #11273, assuming I can make my way around the code. I was just noting that the multiply-connected spiderweb didn't look right - and there it is!

jdemeyer commented 12 years ago

Merged: sage-5.1.beta2