PowerSeriesRing should call Ring.__init__

[simon-king-jena]

We have

sage: A.<a,b> = PowerSeriesRing(QQ)
sage: A._is_category_initialized()
False
sage: isinstance(A,Ring)
True

Since Ring.__init__ initialises the category, it seems that the the base class' initialisation is not called. That also prevents us from removing the alias for _Hom_ in sage.rings.ring - see #12876.

CC: @nthiery @nilesjohnson

Component: categories

Keywords: power series

Author: Simon King

Reviewer: Travis Scrimshaw

Merged: sage-5.5.beta1

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

[nthiery]

comment:1

+1

Just 2 cents: alternatively, it could possibly call Parent.init but passing down the appropriate category.

[simon-king-jena]

comment:2

I think I'll even be getting category and coercion framework right (there was __call__, there was no self.Element, there was no _coerce_map_from_). With the patch I'm preparing:

sage: A.<a> = PowerSeriesRing(QQ)
sage: TestSuite(A).run()
sage: A.<a,b> = PowerSeriesRing(QQ)
sage: TestSuite(A).run()

Need to document the new stuff, though.

[simon-king-jena]

comment:3

PS: I also made the power series rings UniqueRepresentation, getting rid of the custom cache.

[nthiery]

comment:4

Replying to @simon-king-jena:

PS: I also made the power series rings UniqueRepresentation, getting rid of the custom cache.

Neat!

It's kind of you to be going the extra mile!

[simon-king-jena]

comment:5

Replying to @nthiery:

Replying to @simon-king-jena:

PS: I also made the power series rings UniqueRepresentation, getting rid of the custom cache.

Neat!

It's kind of you to be going the extra mile!

No, that's more like going by car. At first, I tried to fix the TestSuite by implementing __reduce__, which didn't work well. Inheriting from UniqueRepresentation did the trick.

[nthiery]

comment:6

No, that's more like going by car. At first, I tried to fix the TestSuite by implementing __reduce__, which didn't work well. Inheriting from UniqueRepresentation did the trick.

:-)

Hopefuly the previous reduce was good enough that there won't be issues with the pickles from the pickle jar.

[simon-king-jena]

comment:7

Replying to @nthiery:

Hopefuly the previous reduce was good enough that there won't be issues with the pickles from the pickle jar.

Well, the pickle jar is doctested, plus it won't break unless I remove the unpickling function used in the old pickles (which I don't).

[simon-king-jena]

Author: Simon King

[simon-king-jena]

Changed keywords from none to power series

[simon-king-jena]

comment:8

Cc to the original author (hi Niles).

The patch implements using the category and coercion framework for uni- and multi-variate power series rings. For me, tests pass (but I have a lot of other patches applied as well, so, better you or the patchbot test it as well).

[simon-king-jena]

comment:9

Any reviewer for making power series rings "nicer", from a categorical point of view?

[nthiery]

comment:10

Hi Simon,

I am not sure I'll get through the review, but I am now browsing through the patch while Sage compiles C3. Here are some little comments:

Near the TestSuite calls, I see things like:

    sage: loads(dumps(X)) is X
    True

This is a generic test, so I'd rather not include those, and if you feel strong about them to add it as a generic test in the TestSuite. Maybe the X._test_pickling() should check whether X has unique representation, and test with is or == accordingly? Or instead one could just check elsewhere that equality is indeed defined in term of is (like testting whether the __eq__ method is that inherited from UniqueRepresentation)

line 362: this should be is_MPowerSeriesRing(S):

        if (is_MPolynomialRing(S) or is_MPowerSeriesRing)

Cheers, Nicolas

[simon-king-jena]

comment:11

Replying to @nthiery:

Near the TestSuite calls, I see things like:

    sage: loads(dumps(X)) is X
    True

This is a generic test, so I'd rather not include those, and if you feel strong about them to add it as a generic test in the TestSuite.

I added it near the TestSuite calls, because it looks like a generic test (I argue below that it isn't really generic), and because the TestSuite does not (and can not, I think) cover it.

Maybe the X._test_pickling() should check whether X has unique representation, and test with is or == accordingly? Or instead one could just check elsewhere that equality is indeed defined in term of is (like testting whether the __eq__ method is that inherited from UniqueRepresentation)

I thought that to TestSuite(X) belong all generic tests that only depend on the category of X. But apparently X.category() will not tell whether X is a unique parent.

Moreover, doing if isinstance(self, UniqueRepresentation) or testing the __eq__ method would certainly not cover all unique parents. Namely some of them are created by a UniqueFactory. In that case, neither the category nor the class give a hint on whether pickling should work with X is loads(dumps(X)).

Just think of polynomial rings: They are unique parents, but that's because of a polynomial ring constructor, and they do have

    cdef int _cmp_c_impl(left, Parent right) except -2:
        if isinstance(right, (MPolynomialRing_libsingular, MPolynomialRing_polydict_domain)):
            return cmp((left.base_ring(), map(str, left.gens()), left.term_order()),
                       (right.base_ring(), map(str, right.gens()), right.term_order()))
        else:
            return cmp(type(left),type(right))

The X is loads(dumps(X)) thus not being generic, I think it clearly does not belong to TestSuite(X). I do feel strongly about having that test in addition TestSuite(X).

line 362: this should be is_MPowerSeriesRing(S):

        if (is_MPolynomialRing(S) or is_MPowerSeriesRing)

Right. And there should obviously be a test where is_MPowerSeriesRing(S) is needed.

[simon-king-jena]

comment:12

Now I am totally puzzled. Apparently my tests for _coerce_map_from_ are all wrong, because the method is inherited from somewhere else!

[simon-king-jena]

comment:13

I see. There are two methods _coerce_map_from_ -- and the second overrides the first (which I wrote).

Since the second (i.e., the old) method seems to be correct, I'll just move my new tests to there.

[simon-king-jena]

Implement category and coercion framework for power series rings

[simon-king-jena]

comment:14

Attachment: trac_13412_category_for_power_series_rings.patch.gz

OK, the patch is updated. I am running doctests now, with

$ hg qa
trac_715_combined.patch
trac_715_local_refcache.patch
trac_715_safer.patch
trac_715_specification.patch
trac_11521_homset_weakcache_combined.patch
trac_11521_callback.patch
13145.patch
trac_13447-sanitise_ring_refcount.patch
trac12215_weak_cached_function-sk.patch
trac12215_segfault_fixes.patch
trac_12313-mono_dict-combined-random-sk.patch
trac_12313_quit_sage.patch
trac13370_deprecate_is_field.patch
trac_13378-convert_map_shortcut.patch
trac_13412_category_for_power_series_rings.patch

[simon-king-jena]

comment:15

Tests work for me.

[tscrim]

Reviewer: Travis Scrimshaw

[tscrim]

comment:16

Everything looks good to me.

[jdemeyer]

Merged: sage-5.5.beta1