Opened 9 years ago

Closed 8 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 nthiery)

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:

  • #5879 (crystals)
  • #5891 categories
  • free modules
  • automatic monoid

~500 lines of doctest for 15 lines of code

Attachments (1)

unique_representation-5120-submitted.patch (23.8 KB) - added by nthiery 8 years ago.

Download all attachments as: .zip

Change History (23)

comment:1 Changed 9 years ago by mhansen

  • Milestone changed from sage-combinat to sage-duplicate/invalid/wontfix
  • Resolution set to duplicate
  • Status changed from new to closed

Duplicate of #5119

comment:2 Changed 9 years ago by mabshoff

  • 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 8 years ago by nthiery

  • 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 8 years ago by cremona

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 8 years ago by was

  • 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 8 years ago by nthiery

  • 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 8 years ago by nthiery

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: Changed 8 years ago by 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.

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

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: Changed 8 years ago by 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

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

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: Changed 8 years ago by nthiery

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 8 years ago by mabshoff

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 8 years ago by nthiery

  • Description modified (diff)

comment:15 Changed 8 years ago by bump

Applies cleanly to sage-3.4.2.rc0 and passes all tests.

comment:16 Changed 8 years ago by mabshoff

  • 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 8 years ago by nthiery

  • 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 8 years ago by nthiery

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 8 years ago by nthiery

Updated patch includes ReST fixes suggested by David Roe, as well as an example on how to handle properly default arguments.

comment:20 Changed 8 years ago by nthiery

  • Status changed from new to assigned

Changed 8 years ago by nthiery

comment:21 Changed 8 years ago by roed

  • 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 8 years ago by mabshoff

  • 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

Note: See TracTickets for help on using tickets.