Clean up linestyle arguments throughout Sage #13834

ppurka · 2012-12-15T20:57:45Z

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

CC: @kcrisman @sagetrac-twch

Component: graphics

Author: Tobias Weich, Punarbasu Purkayastha

Reviewer: Punarbasu Purkayastha, Tobias Weich, Nathann Cohen

Merged: sage-5.11.rc0

Issue created by migration from https://trac.sagemath.org/ticket/13834

The text was updated successfully, but these errors were encountered:

sagetrac-twch · 2013-04-06T20:57:24Z

comment:3

Attachment: trac_13834_enable_short_linestyle.patch.gz

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.

ppurka · 2013-04-07T03:11:10Z

comment:5

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 function get_matplotlib_linestyle. So you can get rid of the try block and import 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'`` ....

sagetrac-twch · 2013-04-08T12:38:44Z

Attachment: tratrac_13834_enable_short_linestyle2.patch.gz

some improvements proposed by ppurka have been added. Can directly be applied to sage 5.8

sagetrac-twch · 2013-04-08T12:44:03Z

comment:6

Hi Basu,

I tried to improve the formatting following your comments and added a new patch.

tobi

sagetrac-twch · 2013-04-08T21:14:08Z

Author: Tobias Weich

ppurka · 2013-04-13T17:36:12Z

comment:8

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

sagetrac-twch · 2013-04-17T14:36:58Z

comment:9

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.

ppurka · 2013-04-17T14:50:08Z

comment:10

Replying to @sagetrac-twch:

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

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. :)

nathanncohen · 2013-06-30T09:39:37Z

comment:11

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

ppurka · 2013-06-30T12:30:38Z

comment:12

The rebased patches doesn't pass doctests in sage-5.11.beta3. This patch needs more TLC.

ppurka · 2013-06-30T12:30:38Z

Work Issues: fix doctests

ppurka · 2013-06-30T12:39:36Z

comment:13

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

ppurka · 2013-06-30T13:29:47Z

Apply to devel/sage (rebased to sage-5.11.beta3)

ppurka · 2013-06-30T13:30:18Z

Attachment: trac_13834_enable_short_linestyle.2.patch.gz

Apply to devel/sage (Rebased to sage-5.11.beta3)

ppurka · 2013-06-30T13:32:08Z

comment:14

Attachment: trac_13834-more_linestyle_cleanups.patch.gz

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

ppurka · 2013-06-30T13:32:08Z

Changed work issues from fix doctests to none

ppurka · 2013-06-30T13:34:30Z

Changed author from Tobias Weich to Tobias Weich, Punarbasu Purkayastha

ppurka · 2013-06-30T13:34:30Z

Reviewer: Punarbasu Purkayastha, Tobias Weich, Nathann Cohen

ppurka · 2013-06-30T13:34:30Z

comment:15

I also removed some trailing whitespace that I didn't see earlier.

nathanncohen · 2013-06-30T15:19:12Z

comment:16

It also passes all tests on my computers except for unrelated things.. Soooooooooo let it go ! Thank you for this patch :-)

Nathann

ppurka · 2013-06-30T15:55:44Z

comment:17

Yay! Thanks. :)

nathanncohen · 2013-07-01T12:28:02Z

comment:18

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

jdemeyer · 2013-07-31T12:54:17Z

Merged: sage-5.11.rc0

ppurka added c: graphics labels Dec 15, 2012

ppurka assigned jasongrout and williamstein Dec 15, 2012

sagetrac-twch mannequin added the s: needs review label Apr 6, 2013

This comment has been minimized.

Sign in to view

ppurka added s: needs work and removed s: needs review labels Jun 30, 2013

This comment has been minimized.

Sign in to view

ppurka added s: needs review and removed s: needs work labels Jun 30, 2013

nathanncohen mannequin added s: positive review and removed s: needs review labels Jun 30, 2013

ppurka added this to the sage-5.11 milestone Jun 30, 2013

ppurka removed the wishlist item label Jun 30, 2013

jdemeyer removed the s: positive review label Jul 31, 2013

jdemeyer closed this as completed Jul 31, 2013

This was referenced Aug 11, 2013

Clean up arguments to all plotting functions #13828

Open

Set individual legend font color in Sage #14580

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up linestyle arguments throughout Sage #13834

Clean up linestyle arguments throughout Sage #13834

ppurka commented Dec 15, 2012

sagetrac-twch mannequin commented Apr 6, 2013

ppurka commented Apr 7, 2013

sagetrac-twch mannequin commented Apr 8, 2013

sagetrac-twch mannequin commented Apr 8, 2013

sagetrac-twch mannequin commented Apr 8, 2013

This comment has been minimized.

ppurka commented Apr 13, 2013

sagetrac-twch mannequin commented Apr 17, 2013

ppurka commented Apr 17, 2013

nathanncohen mannequin commented Jun 30, 2013

ppurka commented Jun 30, 2013

ppurka commented Jun 30, 2013

ppurka commented Jun 30, 2013

ppurka commented Jun 30, 2013

ppurka commented Jun 30, 2013

This comment has been minimized.

ppurka commented Jun 30, 2013

ppurka commented Jun 30, 2013

ppurka commented Jun 30, 2013

ppurka commented Jun 30, 2013

ppurka commented Jun 30, 2013

nathanncohen mannequin commented Jun 30, 2013

ppurka commented Jun 30, 2013

nathanncohen mannequin commented Jul 1, 2013

jdemeyer commented Jul 31, 2013

Clean up linestyle arguments throughout Sage #13834

Clean up linestyle arguments throughout Sage #13834

Comments

ppurka commented Dec 15, 2012

sagetrac-twch mannequin commented Apr 6, 2013

ppurka commented Apr 7, 2013

sagetrac-twch mannequin commented Apr 8, 2013

sagetrac-twch mannequin commented Apr 8, 2013

sagetrac-twch mannequin commented Apr 8, 2013

This comment has been minimized.

ppurka commented Apr 13, 2013

sagetrac-twch mannequin commented Apr 17, 2013

ppurka commented Apr 17, 2013

nathanncohen mannequin commented Jun 30, 2013

ppurka commented Jun 30, 2013

ppurka commented Jun 30, 2013

ppurka commented Jun 30, 2013

ppurka commented Jun 30, 2013

ppurka commented Jun 30, 2013

This comment has been minimized.

ppurka commented Jun 30, 2013

ppurka commented Jun 30, 2013

ppurka commented Jun 30, 2013

ppurka commented Jun 30, 2013

ppurka commented Jun 30, 2013

nathanncohen mannequin commented Jun 30, 2013

ppurka commented Jun 30, 2013

nathanncohen mannequin commented Jul 1, 2013

jdemeyer commented Jul 31, 2013