Ticket #4201 (closed defect: fixed)

Opened 5 years ago

Last modified 5 years ago

[with patch, positive review] add .options and .reset to plot functions

Reported by: mhansen Owned by: mhansen
Priority: major Milestone: sage-3.1.3
Component: graphics Keywords:
Cc: boothby Work issues:
Report Upstream: Reviewers:
Authors: Merged in:
Dependencies: Stopgaps:

Description


Attachments

trac_4201.patch Download (15.3 KB) - added by mhansen 5 years ago.
trac_4201-rebased.patch Download (15.3 KB) - added by jason 5 years ago.
Apply instead of previous patch
trac_4201-reset-doc.patch Download (1.3 KB) - added by mhansen 5 years ago.
trac_4201-options-doc-defaults.patch Download (1.5 KB) - added by mhansen 5 years ago.

Change History

comment:1 Changed 5 years ago by mhansen

  • Cc boothby added
  • Owner changed from was to mhansen
  • Status changed from new to assigned
  • Summary changed from add .options and .reset to plot functions to [with patch, needs review] add .options and .reset to plot functions

comment:2 Changed 5 years ago by mhansen

Note that this patch is on top of #4099.

comment:3 Changed 5 years ago by mabshoff

A couple spelling problems:

  • defualt options
  • decorator whcich renames

Cheers,

Michael

Changed 5 years ago by mhansen

comment:4 Changed 5 years ago by mhansen

What spelling problems? I don't see any ;-)

comment:5 Changed 5 years ago by jason

  • Summary changed from [with patch, needs review] add .options and .reset to plot functions to [with patch, needs rebasing] add .options and .reset to plot functions

This does not apply cleanly to alpha1. For example, the plot_slope_field patch in alpha1 causes a reject.

comment:6 Changed 5 years ago by jason

  • Summary changed from [with patch, needs rebasing] add .options and .reset to plot functions to [with patch, needs review and rebasing] add .options and .reset to plot functions

Changed 5 years ago by jason

Apply instead of previous patch

comment:7 Changed 5 years ago by jason

  • Summary changed from [with patch, needs review and rebasing] add .options and .reset to plot functions to [with patch, needs review] add .options and .reset to plot functions

trac_4201-rebased.patch is rebased against 3.1.3alpha1; apply it instead of the first patch.

comment:8 Changed 5 years ago by jason

This could make #4154 easy now.

comment:9 Changed 5 years ago by jason

  • Summary changed from [with patch, needs review] add .options and .reset to plot functions to [with patch, positive review] add .options and .reset to plot functions

The code looks reasonable, the rebased patch applies cleanly, it seems to work as advertised, and it solves the problem in a very elegant fashion. Oh, and doctests in plot/*.py pass.

Positive review.

comment:10 Changed 5 years ago by jason

  • Summary changed from [with patch, positive review] add .options and .reset to plot functions to [with patch, positive review pending additional docstring] add .options and .reset to plot functions

the third patch above adds a docstring for the reset function. Apply it on top of the second patch.

comment:11 Changed 5 years ago by jason

The trac_4207-options-doc-defaults.patch adds a defaults() method to get the default options and also modifies the reset() and defaults() docstrings to reflect the options of the current function instead of the vague general decorator docstring.

comment:12 Changed 5 years ago by jason

  • Summary changed from [with patch, positive review pending additional docstring] add .options and .reset to plot functions to [with patch, needs review of referee patches] add .options and .reset to plot functions

Positive review for the rebased original patch. There probably ought to be a review of my patches. Mike, do you want to review them?

Changed 5 years ago by mhansen

Changed 5 years ago by mhansen

comment:13 Changed 5 years ago by mhansen

  • Summary changed from [with patch, needs review of referee patches] add .options and .reset to plot functions to [with patch, positive review] add .options and .reset to plot functions

I just indented the examples on Jason's patches. +1 to them. Apply only the last three patches.

comment:14 Changed 5 years ago by mabshoff

  • Status changed from assigned to closed
  • Resolution set to fixed

Merged the last three patches in Sage 3.1.3.alpha2

Note: See TracTickets for help on using tickets.