#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)
Change History (17)
comment:1 follow-up: ↓ 3 Changed 12 years ago by
comment:2 follow-up: ↓ 4 Changed 12 years ago by
- 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 12 years ago by
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 writesage: cmp(l11, l21) in [-1,1] Truesince 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: ↓ 5 Changed 12 years ago by
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: ↓ 6 Changed 12 years ago by
comment:6 in reply to: ↑ 5 Changed 12 years ago by
Replying to mabshoff:
I meant the standard "[with patch, needs review]" :)
Oops, I was sure I had done this!
comment:7 Changed 12 years ago by
- 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 12 years ago by
- 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 12 years ago by
- Status changed from new to assigned
comment:10 Changed 12 years ago by
Has anybody non-combinat signed off on this? Not that I don't trust Anne, but ... ;)
Cheers,
Michael
comment:11 Changed 12 years ago by
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: ↓ 13 Changed 12 years ago by
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 12 years ago by
comment:13 in reply to: ↑ 12 Changed 12 years ago by
Replying to robertwb:
I agree this looks good. The only caveat is that the docstring reads
Therefore, ``o`` does inherit the stringwhere it probably should be "does not."
Oops, indeed! Thanks. Patch updated.
comment:14 Changed 12 years ago by
- Resolution set to fixed
- Status changed from assigned to closed
Merged in 4.0.1.alpha0.
comment:15 Changed 12 years ago by
- Merged in set to 4.0.1.alpha0
- Reviewers set to Anne Schilling, Robert Bradshaw
comment:16 Changed 5 years ago by
- Report Upstream set to N/A
This is broken:
Never check the return value of
cmp
to be -1 or 1, but always writesince the value depends on memory location. I have had to fix this literally dozens of times in doctests.
Cheers,
Michael