Opened 9 years ago

Closed 7 years ago

#11028 closed enhancement (fixed)

More Modular ComplexPlot

Reported by: evanandel Owned by: jason, was
Priority: minor Milestone: sage-5.1
Component: graphics Keywords: complex plot riemann map
Cc: kcrisman, robertwb Merged in: sage-5.1.beta2
Authors: Ethan Van Andel Reviewers: Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by kcrisman)

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 trac-11028-modular-complex-plot.2.3.patch and trac_11028-reviewer.patch.

Attachments (2)

trac-11028-modular-complex-plot.2.3.patch (8.9 KB) - added by evanandel 8 years ago.
Fixed the test failures
trac_11028-reviewer.patch (1.8 KB) - added by kcrisman 7 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 9 years ago by evanandel

  • Status changed from new to needs_review

comment:2 Changed 8 years ago by kcrisman

  • Status changed from needs_review to needs_work

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.

comment:3 Changed 8 years ago by kcrisman

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 :)

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

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?

comment:5 in reply to: ↑ 4 Changed 8 years ago by kcrisman

  • Authors changed from evanandel to Ethan Van Andel
  • Reviewers set to Karl-Dieter Crisman

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.

comment:6 Changed 8 years ago by evanandel

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.

comment:7 Changed 8 years ago by kcrisman

  • Status changed from needs_work to needs_review

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!

comment:8 Changed 8 years ago by kcrisman

  • Description modified (diff)

comment:9 Changed 8 years ago by kcrisman

  • Description modified (diff)

comment:10 Changed 8 years ago by kcrisman

  • Status changed from needs_review to needs_info

A few thoughts:

  • In the first example you move up your definition of f and fprime. But then you define them again. The definitions of f and fprime in the second example can probably be omitted, right?
  • Good catch on the hfs that should have been fprimes.
  • I think that the change to ComplexPlot is a good one, except it is now no longer a complex plot! Now it really is a 'color plot', by initialization! So it allows users how to decide how to represent their data, as you say - but complex or otherwise. Maybe you should make the ColorPlot the main method, and then let a ComplexPlot be the initialization of this with zvalues instead of the color array.
  • I understand the two different mag_to_lightness functions. I still don't understand the difference between the two complex_to_rgb functions. What is it?

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!

comment:11 follow-up: Changed 8 years ago by 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.
  • 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.
  • 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.

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

comment:12 in reply to: ↑ 11 Changed 8 years ago by kcrisman

  • Status changed from needs_info to needs_review

Replying to 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.

comment:13 Changed 8 years ago by kcrisman

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?

Changed 8 years ago by evanandel

Fixed the test failures

comment:14 Changed 8 years ago by evanandel

  • Description modified (diff)

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.

comment:15 Changed 7 years ago by kcrisman

  • Description modified (diff)

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

Changed 7 years ago by kcrisman

comment:16 Changed 7 years ago by kcrisman

  • Status changed from needs_review to positive_review

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!

comment:17 Changed 7 years ago by jdemeyer

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