Opened 7 years ago
Closed 6 years ago
#13834 closed enhancement (fixed)
Clean up linestyle arguments throughout Sage
Reported by: | ppurka | Owned by: | jason, was |
---|---|---|---|
Priority: | major | Milestone: | sage-5.11 |
Component: | graphics | Keywords: | |
Cc: | kcrisman, twch | Merged in: | sage-5.11.rc0 |
Authors: | Tobias Weich, Punarbasu Purkayastha | Reviewers: | Punarbasu Purkayastha, Tobias Weich, Nathann Cohen |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
The linestyle argument is inconsistent across plots, as already pointed out in #13423. It seems this is spread out throughout many Sage components including sage.plot.*
and sage.graphs.graph_plot
.
Apply to devel/sage
Attachments (4)
Change History (23)
comment:1 Changed 6 years ago by
- Cc kcrisman added
comment:2 Changed 6 years ago by
- Cc twch added
Changed 6 years ago by
comment:3 Changed 6 years ago by
comment:4 Changed 6 years ago by
- Status changed from new to needs_review
comment:5 Changed 6 years ago by
Hello, Thanks for looking into this ticket. I will be able to do a more thorough review only later this week. With a casual look at the patch, I have the following comments about the formatting. In general, you can look at the Sage developer's guide on how to format certain things.
- Try to limit the lengths of your lines to about 80 characters. An example of how code can be written to fit within that limit is given in PEP8. Very long lines are hard to read and edit. Long strings can also be broken up like this
ValueError("this is a valueerror for %d" "something something %s.\n" "something else"%(20, 'abc'))
- Don't try to catch the
ValueError
that is raised inside the new functionget_matplotlib_linestyle
. So you can get rid of thetry
block andimport verbose
. - In the docstrings, write the strings inside double backticks like this
- ``linestyle`` - (default: ``'solid'``) The style of the line, which is one of ``'dashed'``, ``'dotted'``, ``'solid'`` ....
Changed 6 years ago by
some improvements proposed by ppurka have been added. Can directly be applied to sage 5.8
comment:6 Changed 6 years ago by
Hi Basu,
I tried to improve the formatting following your comments and added a new patch.
tobi
comment:7 Changed 6 years ago by
comment:8 Changed 6 years ago by
- Description modified (diff)
Hi Tobias, your patch is quite good now. It is quite nice of you to have fixed a lot of docstrings.
I also found a bunch of other places where linestyle needed cleanups. It also affected the new function get_matplotlib_linestyle
. I have uploaded a patch and it needs a review. If you see anything you don't like or if you think I missed something, feel free to point it out! :)
Patchbot apply tratrac_13834_enable_short_linestyle2.patch trac_13834-more_linestyle_cleanups.patch
comment:9 follow-up: ↓ 10 Changed 6 years ago by
Hi
thanks for the extensive improvements!
I did the following in order to test the patches:
$ ./sage --testall --long all tests passed
/sage -docbuild reference html
Built without errors or warnings
checked coverage of changed files: There are only some old missing coverages in hyperbolic_arc and hyperbolic_triangle. But I'm not sure how one should test them reasonably.
The only thing which seems to be a little bit unlogical for me is:
301 Linestyles with ``"default"`` or ``"steps"`` in them should also be 302 properly handled. For instance, matplotlib understands only the short 303 version when ``"steps"`` is used:: 304 305 sage: get_matplotlib_linestyle("default", "short") 306 '' 307 sage: get_matplotlib_linestyle("steps--", "short") 308 'steps--' 309 sage: get_matplotlib_linestyle("steps-predashed", "long") 310 'steps-pre--'
Wouldn't the following be more reasonable: If matplotlib in some functions only understand linestyles "stepsdashed" the function get_matplotlib_linestyle should be called with the "short" keyword (as you do it in lines.py) but the keyword shouldn't be ignored in the function itself. But I don't see that this makes any problems so its just a question.
I also tried to play with the enhanced functions and encountered no further problems.
In any case: Since I'm inexperienced someone more experienced should have a final look in order to confirm the patches.
comment:10 in reply to: ↑ 9 Changed 6 years ago by
Replying to twch:
I did the following in order to test the patches:
$ ./sage --testall --long all tests passed/sage -docbuild reference htmlBuilt without errors or warnings
Thanks for checking this. The patchbot is down, otherwise it would have checked for all of it.
The only thing which seems to be a little bit unlogical for me is:
301 Linestyles with ``"default"`` or ``"steps"`` in them should also be 302 properly handled. For instance, matplotlib understands only the short 303 version when ``"steps"`` is used:: 304 305 sage: get_matplotlib_linestyle("default", "short") 306 '' 307 sage: get_matplotlib_linestyle("steps--", "short") 308 'steps--' 309 sage: get_matplotlib_linestyle("steps-predashed", "long") 310 'steps-pre--'Wouldn't the following be more reasonable: If matplotlib in some functions only understand linestyles "stepsdashed" the function get_matplotlib_linestyle should be called with the "short" keyword (as you do it in lines.py) but the keyword shouldn't be ignored in the function itself. But I don't see that this makes any problems so its just a question.
Since get_matplotlib_linestyle
is an internal function I just made it silently output the correct result even on incorrect input. maptlotlib understands only the short form in those cases and so I think it is safe to output the short form always when we know that there is no alternative.
I also tried to play with the enhanced functions and encountered no further problems.
Thanks for checking them. I had checked some but not all.
In any case: Since I'm inexperienced someone more experienced should have a final look in order to confirm the patches.
Well, let's hope someone comes along and gives the patches a test drive. :)
comment:11 Changed 6 years ago by
Helloooooooooooo !!
There were two rejects when I tried to apply the patches on 5.10.rc1 but short of this it looks good to me. Could you please rebase it ? I will then run all tests and set the ticket to positive_review
:-)
Nathann
comment:12 Changed 6 years ago by
- Status changed from needs_review to needs_work
- Work issues set to fix doctests
The rebased patches doesn't pass doctests in sage-5.11.beta3. This patch needs more TLC.
comment:13 Changed 6 years ago by
Well, it seems a bunch of them fail anyway on vanilla sage-5.11.beta3.
sage -t plot/graphics.py # 1 doctest failed sage -t plot/plot3d/plot3d.py # 4 doctests failed sage -t plot/arrow.py # 1 doctest failed sage -t plot/colors.py # 1 doctest failed
comment:14 Changed 6 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
- Work issues fix doctests deleted
Oops sorry. Sage-5.11b3 was just fine. I was using the wrong sage version to test this directory. Passes all doctests in sage/{graphs,plot}
.
Patchbot apply trac_13834_enable_short_linestyle.2.patch trac_13834-more_linestyle_cleanups.patch
comment:15 Changed 6 years ago by
- Reviewers set to Punarbasu Purkayastha, Tobias Weich, Nathann Cohen
I also removed some trailing whitespace that I didn't see earlier.
comment:16 Changed 6 years ago by
- Status changed from needs_review to positive_review
It also passes all tests on my computers except for unrelated things.. Soooooooooo let it go ! Thank you for this patch :-)
Nathann
comment:18 Changed 6 years ago by
Helloooooooooo guys !! If any of you had a split second to give #14805 a review, it would be very very kind. The patch does absolutely nothing, it's just a doc cleaning of a file that was not included in the documentation ^^;
Thannnks you if you can ! Otherwise it's not a very bad problem :-)
Nathann
comment:19 Changed 6 years ago by
- Merged in set to sage-5.11.rc0
- Resolution set to fixed
- Status changed from positive_review to closed
This patch should enable short matplotlib linestyle keywords like ':' od '--' for arc, arrow, bezier_path, circle and ellipse. Up to now only long linestyle were available.
Furthermore worng linestyle keywords now should lead to a Warning that the linestyle is unknown and ignored but the object is nevertheless plotted with the default linestyle.