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
Improve adaptive rendering in plot() #3813
Comments
Attachment: sage-adaptive-plot.patch.gz Better adaptive rendering |
reviewers changes |
comment:3
Attachment: trac_3813-review.patch.gz slabbe and I have some suggestions that we are submitting as trac_3813-review.patch. Most are documentation edits. Two noteworthy changes:
(sage-adaptive-plot.patch needs to be applied first.) |
comment:4
REVIEW:
Thus you've broken all code that uses plot_division. You need to either support plot_division (why not??), or at the worst put in a deprecation warning. If you do deprecation, see
In particular, if you make p1[0] and p2[0] both Python int's then you can easily get a completely wrong answer. You must coerce the entries of the pi's to floats first or replace 2 by 2.0. For example:
Same comments about
|
comment:5
IMPORTANT: I want to emphasize that I'm not claiming that some of the bugs mentioned today are a result of this patch! If you don't want to fix them or don't know how, just let me know. For example the first patch involving |
comment:6
OH, I realized that I can make a plot that doesn't use adaptive refinement by using adaptive_recursion=0, and that this is clearly documented. |
Attachment: trac_3813_v2.patch.gz |
comment:7
Integrate all feedback and fix all reported issues. This patch is cumulative, so you don't need the first two patches before. |
Attachment: 3813-anakha-adaptive-plot-v3.patch.gz Attachment: 3813-diff.patch.gz |
comment:8
I made some documentation changes and changed the meaning of adaptive_tolerance slightly. Apply only I think this is ready to be applied even if my changes are not appreciated. |
comment:9
I kind of agree with these changes. The only one I have some issues is the adaptive_tolerance change. I had a personal debate on whether making it work like I did and what you did. I decided on my way, so as to have a reasonable default in case nothing was specified and leave full control to the user otherwise. In the end I don't really care either way. |
comment:10
This sounds like a positive review. Thanks for this fantastic improvement to the sage plotting library! |
comment:11
This patch has some slight reject issues:
Please rebase it against 3.1.rc0 once it is out. Cheers, Michael |
Attachment: trac_3813_rebase.patch.gz Rebase of the patch against 3.1.1. Includes all prior patches. |
comment:12
Rebase done. Sorry for the delay. |
comment:13
Merged in Sage 3.1.2.alpha0 |
Attachment: trac_3813_doctestfixes.patch.gz |
comment:14
trac_3813_doctestfixes.patch fixes the following two doctest failures:
It also makes two doctests long. Cheers, Michael |
comment:15
Arnaut, Franco, after discussing this in IRC we thought it might be a good idea to sort out the problems with those two failed doctests before merging this patch. Cheers, Michael |
comment:16
I think this is because I changed the default value of adaptive_recursion in the adaptive_refinement() at the last moment. It seems I forgot to rebuild when running the doctests. So just changing the value for what you get should be enough. Or you can add an adaptive_recursion parameter set to 10 and you should get the same results as in the tests. If you get differing results then something is wrong. |
comment:17
I just realized I forgot to change the summary after the argument I made. Also, just to make it clear, the two patches that are needed for anybody wanting to try this out are trac_3813_rebase.patch and trac_3813_doctestfixes.patch |
comment:18
Attachment: trac_3813-final.patch.gz The latest trac_3813-final.patch should apply to the latest Sage. |
comment:20
Merged trac_3813-final.patch in Sage 3.1.2.alpha1 |
William said at Sage Days 9 that he wanted better adaptive rendering. So I did it.
It actually looks much better by default. Better enough that I don't think users will have to touch plot_points anymore.
And it runs just as fast.
Component: graphics
Issue created by migration from https://trac.sagemath.org/ticket/3813
The text was updated successfully, but these errors were encountered: