Introduce a baseclass for singletons

[Simon King]

Since some while, we have sage.categories.category_singleton.Category_singleton. A singleton is a class that allows the creation of exactly one instance (of any subclass). The technical implementa tion of Category_singleton is roughly like this: In some static __classcall__, test that the given class is a direct subclass of Category_singleton; then create a (the) instance of the class; then set the override the classcall of this class and of the class of the instance (which might be a subclass) by a constant function returning the instance.

I suggest to provide this construction scheme generally, in sage.misc.fast_methods.SingletonClass. The idea is: If one write a class inheriting from SingletonClass then one does not need to think about caching the unique instance or writing a fast hash or writing fast comparison, since this will all be inherited from SingletonClass.

I suggest to use it at least on the rational field, which currently has a custom cache, and its hash method is written in Python.

With the branch that I am going to add, I get

sage: %timeit RationalField()
1000000 loops, best of 3: 269 ns per loop
sage: %timeit hash(QQ)
10000000 loops, best of 3: 134 ns per loop
sage: %timeit QQ==ZZ
10000000 loops, best of 3: 144 ns per loop

With vanilla sage-5.12.b5, it is a lot slower:

sage: %timeit RationalField()
10000 loops, best of 3: 82.4 us per loop
sage: %timeit hash(QQ)
1000000 loops, best of 3: 496 ns per loop
sage: %timeit QQ==ZZ
1000000 loops, best of 3: 878 ns per loop

[Simon King]

It is not "needs review" yet, but I guess it only needs test coverage for the new class.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

[changeset:add22e6]

Add tests for SingletonClass?

[Simon King]

I did not run make ptest yet, but I have added tests for the new SingletonClass and hence change the ticket into "needs review".

[Simon King]

It will be needed to change the expected output of several hash functions in the tests, as the hash of QQ is changed object.__hash__(QQ) with my branch.

[Simon King]

Oops, and much worse: It seems that I forgot to think about unpickling old pickles of QQ!

[Simon King]

Replying to SimonKing:

Oops, and much worse: It seems that I forgot to think about unpickling old pickles of QQ!

I stand corrected: THIS seems to work. But the pickle jar broke.

[Simon King]

I have now clue what is going wrong here.

I tested saving a rational number in vanilla sage, and I could read the pickle after rebuilding Sage with my branch. However, it seems that some old pickles in the pickle jar manage to create a version of the integer ring without calling its __init__. This could be accomplished by directly calling __new__. But is this really done somewhere?

[Nicolas M. Thiéry]

I won't have time to review this ticket, but I am all +1 on progress on this direction. Just one thing: the priority is not as high than #10963, and the two patches are likely to conflict with each other, so I'd recommend basing this one on top of #10963.

Cheers,

Nicolas

[Simon King]

Replying to nthiery:

I won't have time to review this ticket, but I am all +1 on progress on this direction. Just one thing: the priority is not as high than #10963, and the two patches are likely to conflict with each other,

Why? My branch touches sage.misc.fast_methods and sage.rings.rational_field and nothing else. I do not aim (yet!) at changing Category_singleton, although it would make sense in the long run (i.e., after #10963.

[Nicolas M. Thiéry]

Replying to SimonKing:

Why? My branch touches sage.misc.fast_methods and sage.rings.rational_field and nothing else. I do not aim (yet!) at changing Category_singleton, although it would make sense in the long run (i.e., after #10963.

Ah, ok, perfect! Double +1 on this ticket then. Well, as long as it does not eat up review time from you on #10963 :-)

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

[changeset:44de392]

Add Rational.new, to make ancient pickles readable

[Simon King]

I did not run make ptest yet, but at least the new commit makes unpickle_all() work.

[Simon King]

Oops, I forgot: Some hash values are to be updated as well.

[Simon King]

Nice finding: One of the hash tests was in fact wrong, because it was a test for the hash of a different class!

I am about to fix this now, by testing against the definition of the respective hash functions. So, I am not testing against the result on 64bit and 32 bit machines, since such tests don't seem meaningful to me.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

[changeset:86d2b64]

Fix the hash value doctests of Heegner points

[Simon King]

Now I am confident that the tests will work, and that the branch is ready for review.

[Simon King]

There are even more tests that test against the hash value of an instance of the wrong class!

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

[changeset:26132d9]

Fix several more hash tests

[Simon King]

There were several hash tests to fix. In all cases, I stopped comparing against a particular number, because

1. the number depends on the architecture (32- versus 64-bit)
2. the number does not show how the hash function works
3. one should test the result of hash against the hash function of the class being tested---otherwise (and this happened in at least three examples) one might test an instance of the wrong class.

The tests should work now.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

[changeset:8489527]

Use "sorted" in one test on sets

[Simon King]

Replying to SimonKing:

The tests should work now.

All but one test worked. The failing test was testing against the order of elements of a set. The obvious solution was to use "sorted" in the example. Hence, with the branch that I updated a minute ago, all tests pass for me.

[Marc Mezzarobba]

Replying to SimonKing:

However, it seems that some old pickles in the pickle jar manage to create a version of the integer ring without calling its __init__. This could be accomplished by directly calling __new__. But is this really done somewhere?

I'm not sure I understand what you mean: isn't that (calling __new__ but not __init__) how unpickling works in general?

[Marc Mezzarobba]

• I am a little bit skeptical about tying the singleton behaviour to comparison by identity if SingletonClass is meant to be the way to implement singleton classes. Indeed, this makes it virtually unusable for Elements as long as sage keeps abusing == to implement semantic (as opposed to syntaxic) equality. And singleton classes descending from Element have at leat one natural use case, namely well-known constants (infinities, pi...). Could you please comment on this choice?
• I don't like having the same explanations and/or tests repeated in several docstrings. I tried to remove redundancies in c5acb88a and 949b7b2aa. But feel free to revert these commits if you disagree!
• The same goes for 207f6b72, where I applied SingletonClass to other parents (initially to check how usable it was in practice, but since it's done...).
• Finally, I made a few minor changes in d9ca799c. Could you please check that none of the things I "fixed" was intentional?

---

New commits:

​d9ca799	SingletonClass: cosmetic changes.
​c5acb88	SingletonClass: Less redundant docstrings
​949b7b2	QQ/SingletonClass: Delete redundant doctest
​4123a50	Merge in '6.1.rc0' to prevent whitespace conflicts
​207f6b7	Make QQbar, AA, and PariRing 'SingletonClass'es

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

​0f900b9	Use WithEqualityById on SingletonClass, replace QQ.__hash__
​159daaa	Add tests for SingletonClass
​a9d0e5a	Add Rational.__new__, to make ancient pickles readable
​61ab8c8	Fix the hash value doctests of Heegner points
​5c1d17b	Fix several more hash tests
​4969e24	Use "sorted" in one test on sets
​a2f369d	SingletonClass: cosmetic changes.
​59aea3c	SingletonClass: Less redundant docstrings
​765b281	QQ/SingletonClass: Delete redundant doctest
​dfbb8bb	Make QQbar, AA, and PariRing 'SingletonClass'es

[Marc Mezzarobba]

rebased (not fully tested yet, but seems to work)

---

Last 10 new commits:

​0f900b9	Use WithEqualityById on SingletonClass, replace QQ.__hash__
​159daaa	Add tests for SingletonClass
​a9d0e5a	Add Rational.__new__, to make ancient pickles readable
​61ab8c8	Fix the hash value doctests of Heegner points
​5c1d17b	Fix several more hash tests
​4969e24	Use "sorted" in one test on sets
​a2f369d	SingletonClass: cosmetic changes.
​59aea3c	SingletonClass: Less redundant docstrings
​765b281	QQ/SingletonClass: Delete redundant doctest
​dfbb8bb	Make QQbar, AA, and PariRing 'SingletonClass'es

[Travis Scrimshaw]

Perhaps Simon can comment at some point, but here are my thoughts.

Replying to mmezzarobba:

• I am a little bit skeptical about tying the singleton behaviour to comparison by identity if SingletonClass is meant to be the way to implement singleton classes. Indeed, this makes it virtually unusable for Elements as long as sage keeps abusing == to implement semantic (as opposed to syntaxic) equality. And singleton classes descending from Element have at leat one natural use case, namely well-known constants (infinities, pi...). Could you please comment on this choice?

I would say this is okay as you can always override __eq__ as necessary, and python's default is to do equality check by id.

• I don't like having the same explanations and/or tests repeated in several docstrings. I tried to remove redundancies in c5acb88a and 949b7b2aa. But feel free to revert these commits if you disagree!

I moved the pickling test to the class-level doc in order to make it more visible.

• The same goes for 207f6b72, where I applied SingletonClass to other parents (initially to check how usable it was in practice, but since it's done...).
• Finally, I made a few minor changes in d9ca799c. Could you please check that none of the things I "fixed" was intentional?

I think they were typos.

I've also made some other misc review changes. However I have a question, should this be in sage/misc or in sage/structure since it feels more like a structural piece of Sage? Otherwise I'm happy with the current state of things (and found similar speedups).

---

New commits:

​4ad8e71	Merge branch 'u/mmezzarobba/15247-singleton_class-rebased' of trac.sagemath.org:sage into public/structure/singleton_class-15247
​0d78737	Some review changes for #15247.

[Simon King]

Replying to tscrim:

Perhaps Simon can comment at some point, but here are my thoughts.

Well, at some point I should perhaps look at the code/doc again...

I've also made some other misc review changes. However I have a question, should this be in sage/misc or in sage/structure since it feels more like a structural piece of Sage?

At #15820 we had a similar discussion, and Nicolas has convinced me with the following: If the structure being implemented is Sage-specific (examples: elements, parents) then it goes into sage.structure. If the structure being implemented would make sense outside of Sage, then it goes into sage.misc (examples: classcall metaclass, sequences of bounded integers as in #15820).

And I'd say that a singleton baseclass makes sense outside of Sage, hence, should go into sage.misc.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​391421e

Merge branch 'public/singleton_class-15247' of trac.sagemath.org:sage into public/singleton_class-15247

[Travis Scrimshaw]

Replying to SimonKing:

At #15820 we had a similar discussion, and Nicolas has convinced me with the following: If the structure being implemented is Sage-specific (examples: elements, parents) then it goes into sage.structure. If the structure being implemented would make sense outside of Sage, then it goes into sage.misc (examples: classcall metaclass, sequences of bounded integers as in #15820).

Thanks for reminding me of that (and sorry, I let this ticket slip of my radar); Nicolas told me at one time but I couldn't remember which when where.

And I'd say that a singleton baseclass makes sense outside of Sage, hence, should go into sage.misc.

I agree.

---

Rebased and we just need a little bit more review.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​3e9e736

Fix merge error

[Travis Scrimshaw]

ping

Could someone just review my change 0d78737? I've given a positive review to everything else.

[Nicolas M. Thiéry]

I just had a look at 0d78737, and it looks good to me.

I would have two semi-unrelated suggestions though:

• Maybe rename the class to Singleton rather than SingletonClass: then the name of the class will describe, as is usual, what its instances are, rather than the class itself.

• For the test that is modified in 0d78737: make the inheritance explicit by creating a subclasss rather than trigerring such an inheritance through a call to _refine_category.

Thanks!

Cheers,

Nicolas

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​3bd5ae0

Merge branch 'public/singleton_class-15247' of trac.sagemath.org:sage into public/singleton_class-15247

[Travis Scrimshaw]

Done and done.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​aa9821e

Changes from Nicolas' suggestions.

[Nicolas M. Thiéry]

Thank you Travis! I double checked your changes, hence positive review.

[Travis Scrimshaw]

Thank you Nicolas. Also thank you Simon and Marc for your work on this.