Opened 8 years ago

Closed 8 years ago

Last modified 10 months ago

#5967 closed enhancement (fixed)

[with patch, positive review] ElementWrapper: A class for wrapping Sage or Python objects as Sage elements

Reported by: nthiery Owned by: nthiery
Priority: major Milestone: sage-4.0.1
Component: misc Keywords:
Cc: sage-combinat Merged in: 4.0.1.alpha0
Authors: Nicolas M. Thiéry Reviewers: Anne Schilling, Robert Bradshaw
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

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.

Attachments (1)

element_wrapper-5967-submitted.patch (6.4 KB) - added by nthiery 8 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 follow-up: Changed 8 years ago by 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.

Cheers,

Michael

comment:2 follow-up: Changed 8 years ago by mabshoff

  • Summary changed from ElementWrapper: A class for wrapping Sage or Python objects as Sage elements to [with patch, needs work] ElementWrapper: A class for wrapping Sage or Python objects as Sage elements

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

comment:3 in reply to: ↑ 1 Changed 8 years ago by nthiery

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

comment:4 in reply to: ↑ 2 ; follow-up: Changed 8 years ago by nthiery

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

comment:5 in reply to: ↑ 4 ; follow-up: Changed 8 years ago by mabshoff

Replying to nthiery:

Replying to 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

comment:6 in reply to: ↑ 5 Changed 8 years ago by nthiery

Replying to mabshoff:

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

Oops, I was sure I had done this!

comment:7 Changed 8 years ago by nthiery

  • Summary changed from [with patch, needs work] ElementWrapper: A class for wrapping Sage or Python objects as Sage elements to [with patch, needs review] ElementWrapper: A class for wrapping Sage or Python objects as Sage elements

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

comment:8 Changed 8 years ago by aschilling

  • Summary changed from [with patch, needs review] ElementWrapper: A class for wrapping Sage or Python objects as Sage elements to [with patch, positive review] ElementWrapper: A class for wrapping Sage or Python objects as Sage elements

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!

comment:9 Changed 8 years ago by nthiery

  • Status changed from new to assigned

comment:10 Changed 8 years ago by mabshoff

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

Cheers,

Michael

comment:11 Changed 8 years ago by nthiery

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.

comment:12 follow-up: Changed 8 years ago by 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."

Changed 8 years ago by nthiery

comment:13 in reply to: ↑ 12 Changed 8 years ago by nthiery

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.

comment:14 Changed 8 years ago by mhansen

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

Merged in 4.0.1.alpha0.

comment:15 Changed 8 years ago by mhansen

  • Authors set to Nicolas Thiery
  • Merged in set to 4.0.1.alpha0
  • Reviewers set to Anne Schilling, Robert Bradshaw

comment:16 Changed 10 months ago by chapoton

  • Authors changed from Nicolas Thiery to Nicolas M. Thiéry
  • Report Upstream set to N/A
Note: See TracTickets for help on using tickets.