Opened 12 years ago
Closed 11 years ago
#11028 closed enhancement (fixed)
More Modular ComplexPlot
Reported by: | Ethan Van Andel | Owned by: | jason, was |
---|---|---|---|
Priority: | minor | Milestone: | sage-5.1 |
Component: | graphics | Keywords: | complex plot riemann map |
Cc: | Karl-Dieter Crisman, Robert Bradshaw | 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 )
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)
Change History (19)
comment:1 Changed 12 years ago by
Status: | new → needs_review |
---|
comment:2 Changed 12 years ago by
Status: | needs_review → needs_work |
---|
comment:3 Changed 12 years ago by
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: 5 Changed 12 years ago by
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 Changed 12 years ago by
Authors: | evanandel → Ethan Van Andel |
---|---|
Reviewers: | → 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 12 years ago by
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 12 years ago by
Status: | needs_work → 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 12 years ago by
Description: | modified (diff) |
---|
comment:9 Changed 12 years ago by
Description: | modified (diff) |
---|
Apply only trac-11028-modular-complex-plot.2.2.patch.
comment:10 Changed 12 years ago by
Status: | needs_review → needs_info |
---|
A few thoughts:
- In the first example you move up your definition of
f
andfprime
. But then you define them again. The definitions off
andfprime
in the second example can probably be omitted, right? - Good catch on the
hf
s that should have beenfprime
s. - 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 twocomplex_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: 12 Changed 11 years ago by
- 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 Changed 11 years ago by
Status: | needs_info → 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 11 years ago by
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 11 years ago by
Attachment: | trac-11028-modular-complex-plot.2.3.patch added |
---|
Fixed the test failures
comment:14 Changed 11 years ago by
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 11 years ago by
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 11 years ago by
Attachment: | trac_11028-reviewer.patch added |
---|
comment:16 Changed 11 years ago by
Status: | needs_review → 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 11 years ago by
Merged in: | → sage-5.1.beta2 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
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.