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
Coloring odds and ends #6269
Comments
comment:1
Attachment: colors.patch.gz This patch does several closely related things:
None of these need explanation except the last subpoint. Previous behavior was so dark as to obscure tick marks even on the doctest plots, and so light as to make plotting even z squared look pretty vague. This only adds about .02 ms on average to plots, which means that complex_plot(sqrt) takes a little bit longer but complex_plot(zeta) is indistinguishable from before in timing. One can quibble about it, but this makes zeros still recognizable for polynomials while not allowing them to drown out for zeta, and makes anything yielding high moduli easier to view, while incurring a very modest performance hit, which seems in line with the pedagogical point of complex_plot. |
comment:2
My main concern with this is that converting to something to Cython that is not time critical adds unnecessary build time to the Sage library. Doing development on Cython files is a lot more annoying than with Python files. But, I think the other changes are useful. |
comment:3
Replying to @mwhansen:
+1 -- I strongly agree with this. One also can't trace through code William |
comment:4
Okay, these comments seem reasonable. It was still a good learning experience, and it was very impressive how much faster the commands became. Perhaps there could be some suggestions added in developer guide as to when not to use Cython. Anyway, I will try to put up a patch for 1 and 3ab above sometime in the near future! Description and summary changed accordingly. |
This comment has been minimized.
This comment has been minimized.
Author: Karl-Dieter Crisman |
comment:8
I get doctest errors on sage.math and my laptop. They seem to be to do with graph coloring rather than the valuable refactor of the Color stuff. Could these be separated?
|
Reviewer: Nick Alexander |
comment:9
I should have caught this - I only tested files in plot/, forgot about the graphs/ changes... At first I thought that somehow rainbow() was giving only one color to all these graphs (which have n=3) but as far as I can tell rainbow works fine. And (for example) doing
gives the correct answer. For some reason something in the generator (as opposed to list) definition of all_graph_colorings() is causing a problem, but why this wouldn't have cropped up before is puzzling. I will try to look at it soon. |
comment:10
I finally figured out what is going on.
This is because
and in all the above cases I believe the rainbow input is a Python int coming from a range() or len(). Very annoying. The patch trac_6269.patch should now pass graph and plot doctests, at least it does for me. |
Attachment: trac_6269.patch.gz |
comment:11
Looks good to me. |
comment:12
Some hunk failures when applied against Sage 4.1.alpha0:
|
comment:13
After testing the library of Sage 4.1.alpha0, I received the following test failures, as reported at this by sage-devel thread by Robert Miller.
So when rebasing the patch |
comment:14
The patch |
comment:15
Reinstating ncalexan's positive review. |
comment:16
Replying to @sagetrac-mvngu:
But rlm says in the same thread, "These are all due to the removal of graph_isom.pyx, and I will be...". So that should not be the problem. In fact, I am not sure how one can NOT include graph stuff, because the graph theory modules need some of the coloring stuff, e.g. in graph_colorings.py and several other such files there is the line
and that is no longer where rainbow lives. Now, it is true that rainbow lives in the global namespace, so maybe those lines are all redundant, but as it stands they might cause failure at startup because sage.plot.plot doesn't have rainbow anymore. Since sage.plot.plot does import rainbow, maybe that's enough for rainbow to be re-imported into those files from there for now. I don't feel I have enough of an understanding of Python imports to judge all that. But at the very least the import statements should be correct or deleted; it makes no sense to leave in imports of a non-existent function which cause no problems (now that I fixed the Integer thing noted above). |
rebased against Sage 4.1.alpha1 |
comment:17
Attachment: trac_6269-rebased.patch.gz The patch |
Merged: sage-4.1.alpha2 |
It would be nice to have things like hue() and rainbow() be in their own submodule of plot, and there are a few things in complex_plot regarding coloring that need to be fixed.
Component: graphics
Author: Karl-Dieter Crisman
Reviewer: Nick Alexander
Merged: sage-4.1.alpha2
Issue created by migration from https://trac.sagemath.org/ticket/6269
The text was updated successfully, but these errors were encountered: