Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#5997 closed enhancement (fixed)

[with patch, positive review] deprecate the "order" method on elements of rings.

Reported by: was Owned by: tbd
Priority: minor Milestone: sage-4.0
Component: algebra Keywords:
Cc: Merged in: 4.0.alpha0
Authors: John Palmieri Reviewers: Minh Van Nguyen
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

There was a vote in sage-devel in the thread entitled "order of elements in the field" to deprecate the order method on ring elements, and keep just the additive_order and multiplicative_order methods. The order method is too ambiguous in the context of rings, and does cause regular confusion.

This should be officially deprecated, and only removed after >= 6 months.

Attachments (2)

trac_5997.patch (1.1 KB) - added by jhpalmieri 12 years ago.
trac_5997-reviewer.patch (659 bytes) - added by mvngu 12 years ago.
reviewer patch; fix trivial typo

Download all attachments as: .zip

Change History (9)

comment:1 Changed 12 years ago by jhpalmieri

  • Summary changed from deprecate the "order" method on elements of rings. to [with patch, needs review] deprecate the "order" method on elements of rings.

Here's a very simple patch.

comment:2 Changed 12 years ago by mabshoff

Shouldn't this patch add a doctest while adding the deprecation warning? Otherwise it isn't obvious that the function is depreacted from looking at "order?"

Cheers,

Michael

comment:3 Changed 12 years ago by jhpalmieri

Shouldn't this patch add a doctest while adding the deprecation warning?

No, I don't think so... Just kidding. Here's a new patch with a doctest.

Changed 12 years ago by jhpalmieri

Changed 12 years ago by mvngu

reviewer patch; fix trivial typo

comment:4 Changed 12 years ago by mvngu

REFEREE REPORT

The patch trac_5997.patch applies OK against the "post-final" sage-3.4.2 and all doctests pass with the options -t -long. Now say we create an element in Z/12Z like so:

sage: a = Integers(12)(5)

Then both a.order() and a.additive_order() return the same result, but a.order() additionally gives a deprecation warning:

sage: a.order()
/scratch/mvngu/sage-3.4.2-sage.math-only-x86_64-Linux/local/lib/python2.5/site-packages/IPython/iplib.py:2073: DeprecationWarning: The function order is deprecated for ring elements; use additive_order or multiplicative_order instead.
  exec code_obj in self.user_global_ns, self.user_ns
12



However, there's one trivial typo in the patch. This is fixed in the reviewer patch trac_5997-reviewer.patch. Basically, I give positive review to John's patch. Only my patch needs to be reviewed.

comment:5 Changed 12 years ago by mabshoff

  • Summary changed from [with patch, needs review] deprecate the "order" method on elements of rings. to [with patch, positive review] deprecate the "order" method on elements of rings.

Rubber stamp. We don't need no stinkin' review for trivial review patches fixing an obvious typo ;)

Cheers,

Michael

comment:6 Changed 12 years ago by mabshoff

  • Milestone changed from sage-4.0.1 to sage-4.0
  • Resolution set to fixed
  • Status changed from new to closed

Merged both patches in Sage 4.0.alpha0.

Cheers,

Michael

comment:7 Changed 12 years ago by davidloeffler

  • Authors set to John Palmieri
  • Merged in set to 4.0.alpha0
  • Reviewers set to Minh Van Nguyen
Note: See TracTickets for help on using tickets.