Opened 9 years ago
Closed 9 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)
Change History (18)
comment:1 Changed 9 years ago by
comment:2 Changed 9 years ago by
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: ↓ 4 Changed 9 years ago by
PS: I also made the power series rings UniqueRepresentation
, getting rid of the custom cache.
comment:4 in reply to: ↑ 3 ; follow-up: ↓ 5 Changed 9 years ago by
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: ↓ 6 Changed 9 years ago by
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: ↓ 7 Changed 9 years ago by
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 fromUniqueRepresentation
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 9 years ago by
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 9 years ago by
- 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 9 years ago by
Any reviewer for making power series rings "nicer", from a categorical point of view?
comment:10 follow-up: ↓ 11 Changed 9 years ago by
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 9 years ago by
Replying to nthiery:
Near the TestSuite? calls, I see things like:
sage: loads(dumps(X)) is X TrueThis 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 ofis
(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 9 years ago by
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 9 years ago by
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.
comment:14 Changed 9 years ago by
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:16 Changed 9 years ago by
- 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 9 years ago by
- Merged in set to sage-5.5.beta1
- Resolution set to fixed
- Status changed from positive_review to closed
+1
Just 2 cents: alternatively, it could possibly call Parent.init but passing down the appropriate category.