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 ppurka)

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

  1. trac_13834_enable_short_linestyle.2.patch
  2. trac_13834-more_linestyle_cleanups.patch

Attachments (4)

trac_13834_enable_short_linestyle.patch (19.8 KB) - added by twch 6 years ago.
tratrac_13834_enable_short_linestyle2.patch (21.3 KB) - added by twch 6 years ago.
some improvements proposed by ppurka have been added. Can directly be applied to sage 5.8
trac_13834_enable_short_linestyle.2.patch (21.3 KB) - added by ppurka 6 years ago.
Apply to devel/sage (rebased to sage-5.11.beta3)
trac_13834-more_linestyle_cleanups.patch (32.8 KB) - added by ppurka 6 years ago.
Apply to devel/sage (Rebased to sage-5.11.beta3)

Download all attachments as: .zip

Change History (23)

comment:1 Changed 6 years ago by kcrisman

  • Cc kcrisman added

comment:2 Changed 6 years ago by twch

  • Cc twch added

Changed 6 years ago by twch

comment:3 Changed 6 years ago by twch

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.

comment:4 Changed 6 years ago by twch

  • Status changed from new to needs_review

comment:5 Changed 6 years ago by ppurka

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.

  1. 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'))
    
  2. 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.
  3. 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 twch

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

comment:6 Changed 6 years ago by twch

Hi Basu,

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

tobi

comment:7 Changed 6 years ago by twch

  • Authors set to Tobias Weich

comment:8 Changed 6 years ago by ppurka

  • 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: Changed 6 years ago by twch

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 ppurka

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

comment:11 Changed 6 years ago by ncohen

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 ppurka

  • 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 ppurka

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

Changed 6 years ago by ppurka

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

Changed 6 years ago by ppurka

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

comment:14 Changed 6 years ago by ppurka

  • 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 ppurka

  • Authors changed from Tobias Weich to Tobias Weich, Punarbasu Purkayastha
  • 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 ncohen

  • 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:17 Changed 6 years ago by ppurka

  • Milestone changed from sage-wishlist to sage-5.11

Yay! Thanks. :)

comment:18 Changed 6 years ago by ncohen

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 jdemeyer

  • Merged in set to sage-5.11.rc0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.