Opened 9 years ago
Closed 9 years ago
#5120 closed enhancement (fixed)
[with patch, positive review] An alternative implementation of the Unique Representation design pattern
Reported by: | nthiery | Owned by: | nthiery |
---|---|---|---|
Priority: | major | Milestone: | sage-4.0 |
Component: | misc | Keywords: | unique representation |
Cc: | sage-combinat | Merged in: | |
Authors: | Reviewers: | ||
Report Upstream: | Work issues: | ||
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
Class deriving from sage.structure.UniqueRepresentation? inherit a unique representation behavior for their instances (and consistent default implementations of equality testing, copying, pickling, ...).
See the documentation for a brief discussion of the relative merits of UniqueRepresentation?? and UniqueFactory??.
As a prerequisite, this patch implements sage.misc.ClasscallMetaclass?, a (trivial) metaclass for customizing class calls via a static method of the class.
This class is used extensively in upcoming sage-combinat patches:
~500 lines of doctest for 15 lines of code
Attachments (1)
Change History (23)
comment:1 Changed 9 years ago by
- Milestone changed from sage-combinat to sage-duplicate/invalid/wontfix
- Resolution set to duplicate
- Status changed from new to closed
comment:2 Changed 9 years ago by
- Milestone changed from sage-duplicate/invalid/wontfix to sage-3.4.1
- Resolution duplicate deleted
- Status changed from closed to reopened
Well, Nicolas closed #5119 as a dupe, so I am reopening this one since I assume one ticket ought to stay open :)
Cheers,
Michael
comment:3 Changed 9 years ago by
- Description modified (diff)
- Owner changed from cwitty to nthiery
- Priority changed from minor to major
- Status changed from reopened to new
- Summary changed from abstract class for unique representation to abstract class for unique representation [with patch, needs review]
comment:4 Changed 9 years ago by
I'm sure there are good reasons why this is needed, but I think it would be helpful to potential reviewers of you could give a real-life example where this class will be used!
comment:5 Changed 9 years ago by
- Summary changed from abstract class for unique representation [with patch, needs review] to [with patch, needs work] abstract class for unique representation
1) The first line of one of the files is:
ï»¿r"""
I.e., lots of weird corrupted characters.
2) There are no doctests for any of the actual functions you defined. Code can't go into sage without 100% doctest coverage of each new function.
comment:6 Changed 9 years ago by
- Cc sage-combinat added
- Description modified (diff)
- Summary changed from [with patch, needs work] abstract class for unique representation to [with patch, needs review] An alternative implementation of the Unique Representation design pattern
comment:7 Changed 9 years ago by
Uploaded an updated patch which fixes 1) and 2)
Ok to abide to a rule, even in a corner case like this where the extra doc really does not add any strength to the test suite, while actually making the code less readable. But it's frustrating to get complained at about documentation for a patch which has a doc/code ratio of 15/1. Next time a please will work better than a gun.
Grumpy O' Pa :-)
comment:8 follow-up: ↓ 9 Changed 9 years ago by
The patch doesn't apply cleanly to sage-3.4.1 since the hunk in sage/structure/all.py needs to be corrected by hand.
comment:9 in reply to: ↑ 8 ; follow-up: ↓ 10 Changed 9 years ago by
Replying to bump:
The patch doesn't apply cleanly to sage-3.4.1 since the hunk in sage/structure/all.py needs to be corrected by hand.
Thanks for the notice. The patch on sage-combinat has been rebased. I'll try to upload it today, after folding in two little other improvements.
comment:10 in reply to: ↑ 9 ; follow-up: ↓ 11 Changed 9 years ago by
Replying to nthiery:
Thanks for the notice. The patch on sage-combinat has been rebased. I'll try to upload it today, after folding in two little other improvements.
I just updated the patch, rebased for 3.4.1, with description header, default implementation of copy/deepcopy, and pickling by reduce rather than getnewargs.
This later change is debatable. For some reason the reduce way was preferable for some application to categories, but I badly enough did not take notes about why
comment:11 in reply to: ↑ 10 ; follow-up: ↓ 12 Changed 9 years ago by
Replying to nthiery:
Replying to nthiery:
Thanks for the notice. The patch on sage-combinat has been rebased. I'll try to upload it today, after folding in two little other improvements.
I just updated the patch, rebased for 3.4.1, with description header, default implementation of copy/deepcopy, and pickling by reduce rather than getnewargs.
This later change is debatable. For some reason the reduce way was preferable for some application to categories, but I badly enough did not take notes about why
Ah, I know why: keyword arguments. See updated, 100% doctested and proofread patch.
comment:12 in reply to: ↑ 11 ; follow-up: ↓ 13 Changed 9 years ago by
Ah, I know why: keyword arguments. See updated, 100% doctested and proofread patch.
Oops trivial update to apply cleanly on 3.4.1. Thanks Dan for the notice.
Michael: could we change the milestone to 3.4.2?
comment:13 in reply to: ↑ 12 Changed 9 years ago by
Replying to nthiery:
I deleted the old patch.
Michael: could we change the milestone to 3.4.2?
The assignment of milestones is generally insignificant (an exception is like right now when 3.4.2.rc0 was the last merge release and we are in blocker fixes only mode), but as long as this ticket would have gotten a positive review it would have had a chance to go into 3.4.2 regardless which milestone it would have been assigned to.
This patch is also a new design pattern which warrants to be mentioned on sage-devel. It seems to be very well documented and AFAIK it should be properly covered :)
Cheers,
Michael
comment:14 Changed 9 years ago by
- Description modified (diff)
comment:15 Changed 9 years ago by
Applies cleanly to sage-3.4.2.rc0 and passes all tests.
comment:16 Changed 9 years ago by
- Summary changed from [with patch, needs review] An alternative implementation of the Unique Representation design pattern to [with patch, needs work] An alternative implementation of the Unique Representation design pattern
Note that #5879 exposes a problem with this patch. To quote Anne:
I just talked to Nicolas about the pickling problem; this is a shortcoming of the current unique representation patch and he will try to find a solution to the problem in patch 5120.
I will mark this "needs work" until this issue is resolved.
Cheers,
Michael
comment:17 Changed 9 years ago by
- Description modified (diff)
- Keywords unique representation added
- Summary changed from [with patch, needs work] An alternative implementation of the Unique Representation design pattern to [with patch, needs review] An alternative implementation of the Unique Representation design pattern
comment:18 Changed 9 years ago by
I changed a bit the underlying implementation which fixes the code and should be more robust. I also added 2 pages of doc, and extracted a trivial yet general purpose CallclassMetaclass? (Florent will be interested). Updated patch upload in a couple minutes.
comment:19 Changed 9 years ago by
Updated patch includes ReST fixes suggested by David Roe, as well as an example on how to handle properly default arguments.
comment:20 Changed 9 years ago by
- Status changed from new to assigned
Changed 9 years ago by
comment:21 Changed 9 years ago by
- Summary changed from [with patch, needs review] An alternative implementation of the Unique Representation design pattern to [with patch, positive review] An alternative implementation of the Unique Representation design pattern
I'd like to see some more thought go into what use cases are best served by UniqueRepresentation? and what by UniqueFactory?. But the code is well documented, does what it claims to do, and UniqueRepresentation? is easier to use than UniqueFactory?. So positive review.
comment:22 Changed 9 years ago by
- Milestone changed from sage-4.0.1 to sage-4.0
- Resolution set to fixed
- Status changed from assigned to closed
Merged in Sage 4.0.rc0.
Cheers,
Michael
Duplicate of #5119