Ticket #6367 (closed defect: fixed)

Opened 4 years ago

Last modified 6 months ago

polygon2d -- several issues: typo in docs, shouldn't have been renamed

Reported by: was Owned by: was
Priority: major Milestone: sage-5.4.1
Component: graphics Keywords:
Cc: ksmith Work issues:
Report Upstream: N/A Reviewers: Volker Braun
Authors: Karl-Dieter Crisman, Kenneth Smith Merged in: sage-5.4.1.rc0
Dependencies: Stopgaps:

Description

The help for polygon2d? says:

 Type ``polygon.options`` for a dictionary of the default 
    options for polygons.  You can change this to change 

but it should say

 Type ``polygon2d.options`` for a dictionary of the default 
    options for polygons.  You can change this to change 

And for the record I think it is unfortunate that somebody changed the function name from polygon to polygon2d, since that it inconsistent with almost all the rest of plotting in Sage, where the 2d version of the function doesn't specifically say it is 2d (e.g., plot, line, text, etc.) Well, there is line2d, to add to the confusion. The design of Sage graphics is *supposed* to follow the naming scheme in Matheamtica. In Mathematica there is Polygon and Polygon3D. That's what Sage should have.

To resolve this patch, either fix the docstring or change the name back to polygon (not polygon2d). I prefer the latter for consistency with the rest fo the design of sage 2d graphics.

Attachments

trac_6367-polygon.patch Download (1.4 KB) - added by kcrisman 12 months ago.

Change History

comment:1 Changed 3 years ago by kcrisman

  • Report Upstream set to N/A

For the record, this caused some actual problems for a potential Sage user recently, so it should get fixed - and soon. Probably we'll need to make polygon=polygon2d (for 2d input) for deprecation reasons, for now.

comment:2 follow-up: ↓ 3 Changed 12 months ago by kcrisman

  • Status changed from new to needs_review
  • Authors set to Karl-Dieter Crisman

Unfortunately this is pretty hard to change since polygon is expected to work for two and three D input. This patch does take care of the problem.

Changed 12 months ago by kcrisman

comment:3 in reply to: ↑ 2 Changed 12 months ago by kcrisman

Unfortunately this is pretty hard to change since polygon is expected to work for two and three D input.

What I mean, of course, is that it would be hard to change since users probably are using it this way a lot and the deprecation period and all simply isn't worth the effort at this point. Anyway, needs review.

comment:4 Changed 12 months ago by kcrisman

  • Cc ksmith added
  • Authors changed from Karl-Dieter Crisman to Karl-Dieter Crisman, Kenneth Smith

Oops, totally forgot about #12214, which is a dup of this. Given that #12214 is virtually the same ticket, this one is much older, and the patch is quite similar, I'm adding the author there to this ticket and closing that one.

comment:5 Changed 7 months ago by vbraun

  • Status changed from needs_review to positive_review
  • Reviewers set to Volker Braun

Looks good to me!

comment:6 Changed 7 months ago by jdemeyer

  • Milestone changed from sage-5.4 to sage-5.4.1

comment:7 Changed 6 months ago by jdemeyer

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