Opened 7 years ago

Closed 7 years ago

#13412 closed defect (fixed)

PowerSeriesRing should call Ring.__init__

Reported by: SimonKing Owned by:
Priority: major Milestone: sage-5.5
Component: categories Keywords: power series, coercion, category
Cc: nthiery, niles Merged in: sage-5.5.beta1
Authors: Simon King Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

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.

Attachments (1)

trac_13412_category_for_power_series_rings.patch (20.8 KB) - added by SimonKing 7 years ago.
Implement category and coercion framework for power series rings

Download all attachments as: .zip

Change History (18)

comment:1 Changed 7 years ago by nthiery

+1

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

comment:2 Changed 7 years ago by SimonKing

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.

comment:3 follow-up: Changed 7 years ago by SimonKing

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

comment:4 in reply to: ↑ 3 ; follow-up: Changed 7 years ago by nthiery

Replying to SimonKing:

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!

comment:5 in reply to: ↑ 4 ; follow-up: Changed 7 years ago by SimonKing

Replying to nthiery:

Replying to SimonKing:

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.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 7 years ago by nthiery

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.

comment:7 in reply to: ↑ 6 Changed 7 years ago by SimonKing

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

comment:8 Changed 7 years ago by SimonKing

  • Authors set to Simon King
  • Cc niles added
  • Keywords power series coercion category added
  • Status changed from new to needs_review

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

comment:9 Changed 7 years ago by SimonKing

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

comment:10 follow-up: Changed 7 years ago by nthiery

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

comment:11 in reply to: ↑ 10 Changed 7 years ago by SimonKing

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.

comment:12 Changed 7 years ago by SimonKing

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

comment:13 Changed 7 years ago by SimonKing

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.

Changed 7 years ago by SimonKing

Implement category and coercion framework for power series rings

comment:14 Changed 7 years ago by SimonKing

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

comment:15 Changed 7 years ago by SimonKing

  • Owner changed from nthiery to (none)

Tests work for me.

comment:16 Changed 7 years ago by tscrim

  • Milestone changed from sage-5.4 to sage-5.5
  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

Everything looks good to me.

comment:17 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.5.beta1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.