New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
More Modular ComplexPlot #11028
Comments
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 Will we need to introduce a deprecation period for the change to initializing with Also, what is the situation with the |
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 |
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? |
Reviewer: Karl-Dieter Crisman |
Changed author from evanandel to Ethan Van Andel |
comment:5
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.
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
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
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! |
This comment has been minimized.
This comment has been minimized.
comment:9
Apply only attachment: trac-11028-modular-complex-plot.2.2.patch. |
This comment has been minimized.
This comment has been minimized.
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! |
comment:11
Those are my thoughts on the issues raised. Does that clear things up? |
comment:12
Replying to @sagetrac-evanandel:
Ok.
Well, that's true.
Ok, I guess my usual plan is to push refactoring to later, too :)
Yes. So I'll try to review this, and then finish #11273. |
comment:13
At SD35.5, we found out that tests don't pass for some reason dealing with |
Attachment: trac-11028-modular-complex-plot.2.3.patch.gz Fixed the test failures |
This comment has been minimized.
This comment has been minimized.
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. |
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 Apply trac-11028-modular-complex-plot.2.3.patch and trac_11028-reviewer.patch |
This comment has been minimized.
This comment has been minimized.
Attachment: trac_11028-reviewer.patch.gz |
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! |
Merged: sage-5.1.beta2 |
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
The text was updated successfully, but these errors were encountered: