ElementWrapper: A class for wrapping Sage or Python objects as Sage elements

[nthiery]

This patch implements a simple class ElementWrapper for wrapping Sage
or Python objects as Sage elements, with reasonable default
implementations of repr, cmp, hash, etc. The typical use case is for
trivially constructing new element classes from preexisting Sage or
Python classes, with a containment relation.

This class is used extensively in the examples of the upcoming category framework patch #5891.

CC: @sagetrac-sage-combinat

Component: misc

Author: Nicolas M. Thiéry

Reviewer: Anne Schilling, Robert Bradshaw

Merged: 4.0.1.alpha0

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

[sagetrac-mabshoff]

comment:1

This is broken:

 	129	            sage: cmp(l11, l12), cmp(l12, l11)   # values differ 
 	130	            (-1, 1) 
 	131	            sage: cmp(l11, l21), cmp(l21, l11)   # parents differ 
 	132	            (-1, 1) 

Never check the return value of cmp to be -1 or 1, but always write

sage: cmp(l11, l21) in [-1,1]
True

since the value depends on memory location. I have had to fix this literally dozens of times in doctests.

Cheers,

Michael

[sagetrac-mabshoff]

comment:2

This ticket also needs to be properly market with a marker so it is picked up for review.

Another thing to fix is to get this file into the documentation. Unless it is in the documentation the vast majority of people will never know of its existence. Current policy is that every file that is well documented and 100% doctested belongs in the documentation.

Cheers,

Michael

[nthiery]

comment:3

Replying to @sagetrac-mabshoff:

This is broken:

 	129	            sage: cmp(l11, l12), cmp(l12, l11)   # values differ 
 	130	            (-1, 1) 
 	131	            sage: cmp(l11, l21), cmp(l21, l11)   # parents differ 
 	132	            (-1, 1) 

Never check the return value of cmp to be -1 or 1, but always write

sage: cmp(l11, l21) in [-1,1]
True

since the value depends on memory location. I have had to fix this literally dozens of times in doctests.

Ok, will do.

[nthiery]

comment:4

Replying to @sagetrac-mabshoff:

This ticket also needs to be properly market with a marker so it is picked up for review.

Any suggestion for that marker?

Another thing to fix is to get this file into the documentation. Unless it is in the documentation the vast majority of people will never know of its existence. Current policy is that every file that is well documented and 100% doctested belongs in the documentation.

Ok, will do.

[sagetrac-mabshoff]

comment:5

Replying to @nthiery:

Replying to @sagetrac-mabshoff:

This ticket also needs to be properly market with a marker so it is picked up for review.

Any suggestion for that marker?

I meant the standard "[with patch, needs review]" :)

Cheers,

Michael

[nthiery]

comment:6

Replying to @sagetrac-mabshoff:

I meant the standard "[with patch, needs review]" :)

Oops, I was sure I had done this!

[nthiery]

comment:7

Done, and patch updated. Thanks Michael for your suggestions.

[anneschilling]

comment:8

The tests of the patch pass when applied to sage-3.4.2.

Besides the application to the category framework #5891,
this patch will be useful for crystals. For example the
class Letter(Element):
in /combinat/crystals/letters.py
can be shortened by inheriting from ElementWrapper.

I give this patch a positive review!

[sagetrac-mabshoff]

comment:10

Has anybody non-combinat signed off on this? Not that I don't trust Anne, but ... ;)

Cheers,

Michael

[nthiery]

comment:11

The uploaded patch adds a warning about the probable little change of interface in the near future (but after integration of the category patch).
Robert promised to have a look at this shortly.

[robertwb]

comment:12

I agree this looks good. The only caveat is that the docstring reads

Therefore, ``o`` does inherit the string

where it probably should be "does not."

[nthiery]

Attachment: element_wrapper-5967-submitted.patch.gz

[nthiery]

comment:13

Replying to @robertwb:

I agree this looks good. The only caveat is that the docstring reads

Therefore, ``o`` does inherit the string

where it probably should be "does not."

Oops, indeed! Thanks. Patch updated.

[mwhansen]

comment:14

Merged in 4.0.1.alpha0.

[mwhansen]

Reviewer: Anne Schilling, Robert Bradshaw

[mwhansen]

Author: Nicolas Thiery

[mwhansen]

Merged: 4.0.1.alpha0

[fchapoton]

Changed author from Nicolas Thiery to Nicolas M. Thiéry