An alternative implementation of the Unique Representation design pattern

[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:

• Added crystal of letters for type E #5879 (crystals)
• Categories for the working mathematics programmer #5891 categories
• free modules
• automatic monoid

~500 lines of doctest for 15 lines of code

CC: @sagetrac-sage-combinat

Component: misc

Keywords: unique representation

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

[mwhansen]

comment:1

Duplicate of #5119

[sagetrac-mabshoff]

comment:2

Well, Nicolas closed #5119 as a dupe, so I am reopening this one since I assume one ticket ought to stay open :)

Cheers,

Michael

[JohnCremona]

comment:4

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!

[williamstein]

comment:5

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.

[nthiery]

comment:7

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 :-)

[dwbump]

comment:8

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.

[nthiery]

comment:9

Replying to @dwbump:

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.

[nthiery]

comment:10

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

[nthiery]

comment:11

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.

[nthiery]

comment:12

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?

[sagetrac-mabshoff]

comment:13

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

[dwbump]

comment:15

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

[sagetrac-mabshoff]

comment:16

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

[nthiery]

Changed keywords from none to unique representation

[nthiery]

comment:18

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.

[nthiery]

comment:19

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

[nthiery]

Attachment: unique_representation-5120-submitted.patch.gz

[roed314]

comment:21

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.

[sagetrac-mabshoff]

comment:22

Merged in Sage 4.0.rc0.

Cheers,

Michael