#13619 closed enhancement (fixed)
Enable polynomial content over padic fields
Reported by:  saraedum  Owned by:  roed 

Priority:  major  Milestone:  sage7.2 
Component:  padics  Keywords:  sd59, days71 
Cc:  Merged in:  
Authors:  Julian Rüth  Reviewers:  Jeroen Demeyer, Aly Deines 
Report Upstream:  N/A  Work issues:  
Branch:  880984a (Commits)  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
Currently, one cannot call content()
for a polynomial defined over Qp
:
sage: K = Qp(3) sage: R.<t> = K[] sage: t.content() TypeError: ground ring is a field. Answer is only defined up to units.
The intention is apparently to protect the user from calling content()
if he mistakenly got from Zp
into Qp
, since the content there would be always zero or one. However, this can be annoying when writing algorithms which work over Zp
and Qp
but which have to take the content into account over Zp
.
Additionally, the content shows a somewhat strange behaviour for zero polynomials::
sage: R = Zp(3) sage: S.<t> = R[] sage: f = S(R(0,3)); f (O(3^3)) sage: f.is_zero() True sage: f.content() 3 + O(3^23) sage: _.is_zero() False
I don't think that the current behaviour is mathematically incorrect, but I believe it's not very intuitive.
Attachments (1)
Change History (34)
comment:1 Changed 7 years ago by
comment:2 Changed 7 years ago by
 Description modified (diff)
comment:3 Changed 7 years ago by
 Description modified (diff)
Changed 7 years ago by
comment:4 Changed 7 years ago by
comment:5 Changed 7 years ago by
As a quick informal review, the OUTPUT:
block is indented one too many times.
comment:6 Changed 6 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:7 Changed 6 years ago by
 Branch set to u/niles/ticket/13619
 Created changed from 10/19/12 00:56:41 to 10/19/12 00:56:41
 Modified changed from 08/13/13 15:35:53 to 08/13/13 15:35:53
comment:8 Changed 6 years ago by
 Commit set to 3ace36a14641ac23fc97d566905f21e562df5c4b
rebased and converted to git branch; changed indentation of OUTPUT:
block, but no other changes
New commits:
3ace36a  Trac #13619: Enable content for polynomials defined over Qp.

comment:9 Changed 6 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:10 Changed 6 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:11 Changed 5 years ago by
 Branch changed from u/niles/ticket/13619 to u/saraedum/ticket/13619
 Modified changed from 05/06/14 15:20:58 to 05/06/14 15:20:58
comment:12 Changed 5 years ago by
 Keywords sd59 added
 Status changed from new to needs_review
comment:13 Changed 5 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:14 followup: ↓ 16 Changed 5 years ago by
 Commit changed from 3ace36a14641ac23fc97d566905f21e562df5c4b to e57bdb57f22542a6fe81a935be6ea8cc95d270e6
Is there are reason you are only doing this for capped_relative_dense
rings and not in the generic Polynomial_padic
?
New commits:
e57bdb5  Merge branch 'develop' into ticket/13619

comment:15 followup: ↓ 17 Changed 5 years ago by
 Milestone changed from sage6.4 to sage6.5
 Priority changed from trivial to major
 Reviewers set to Jeroen Demeyer
 Status changed from needs_review to needs_work
This should be done for generic padics polynomials. I would prefer to get the same answer for Zp
and Qp
, i.e. p^k
where k
is the minimal valuation. That would be more useful than always getting 1
as answer over Qp
.
comment:16 in reply to: ↑ 14 Changed 5 years ago by
Replying to jdemeyer:
Is there are reason you are only doing this for
capped_relative_dense
rings and not in the genericPolynomial_padic
?
I believe Polynomial_padic
did not exist when I wrote this first. Anyway, there is no reason, I'll change it.
comment:17 in reply to: ↑ 15 ; followups: ↓ 19 ↓ 20 Changed 5 years ago by
Replying to jdemeyer:
This should be done for generic padics polynomials. I would prefer to get the same answer for
Zp
andQp
, i.e.p^k
wherek
is the minimal valuation. That would be more useful than always getting1
as answer overQp
.
This would be inconsistent with the current behaviour of content()
over QQ
. What should be the answer if a coefficient has negative valuation? I would rather stick to the usual definition of content. If people are interested in the content over Zp
, then they should change the base ring.
comment:18 Changed 5 years ago by
Btw. I'd rather return the content as an ideal, just like QQ
does. But this could probably break some code. Do you think this would be an acceptable change?
comment:19 in reply to: ↑ 17 ; followup: ↓ 21 Changed 5 years ago by
I think it's certainly not a good idea that content()
sometimes returns an element and sometimes returns an ideal. Ideally, there would be two methods:
content()
returning an ideal in the mathematical sensecontent_gen()
returning an element, preferably a generator of the fractional ideal generated by the coefficients.
comment:20 in reply to: ↑ 17 ; followup: ↓ 22 Changed 5 years ago by
Replying to saraedum:
What should be the answer if a coefficient has negative valuation?
p^k
where k
is the minimal valuation of the coefficients.
I would rather stick to the usual definition of content.
The "usual" definition doesn't care about the unit part, so answering p^k
is a more useful and equally correct answer over Qp
.
comment:21 in reply to: ↑ 19 Changed 5 years ago by
Replying to jdemeyer:
I think it's certainly not a good idea that
content()
sometimes returns an element and sometimes returns an ideal. Ideally, there would be two methods:
content()
returning an ideal in the mathematical sensecontent_gen()
returning an element, preferably a generator of the fractional ideal generated by the coefficients.
What is the value of content_gen()
? It is almost always equivalent to content().gen()
unless in some cases where both should raise an error?
comment:22 in reply to: ↑ 20 Changed 5 years ago by
Replying to jdemeyer:
Replying to saraedum:
What should be the answer if a coefficient has negative valuation?
p^k
wherek
is the minimal valuation of the coefficients.I would rather stick to the usual definition of content.
The "usual" definition doesn't care about the unit part, so answering
p^k
is a more useful and equally correct answer overQp
.
Ok. You are right. I'll change it.
comment:23 Changed 5 years ago by
 Commit changed from e57bdb57f22542a6fe81a935be6ea8cc95d270e6 to 55e1b96cface91d3bd333c52ee868c668299e1f0
comment:24 Changed 5 years ago by
 Status changed from needs_work to needs_review
comment:26 Changed 4 years ago by
 Commit changed from 55e1b96cface91d3bd333c52ee868c668299e1f0 to 355c1441df16255246733eb0808f43b420264b9c
Branch pushed to git repo; I updated commit sha1. New commits:
355c144  Merge branch 'develop' into t/13619/ticket/13619

comment:27 Changed 4 years ago by
 Commit changed from 355c1441df16255246733eb0808f43b420264b9c to 880984aa7afbb49fff546e7163e30324a953df98
Branch pushed to git repo; I updated commit sha1. New commits:
880984a  Move capped relative content() implementation to the generic padic polynomial class.

comment:28 Changed 4 years ago by
 Status changed from needs_work to needs_review
comment:29 Changed 4 years ago by
 Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, Aly Deines
 Status changed from needs_review to positive_review
Looks good to me.
comment:30 Changed 4 years ago by
 Branch changed from u/saraedum/ticket/13619 to 880984aa7afbb49fff546e7163e30324a953df98
 Resolution set to fixed
 Status changed from positive_review to closed
comment:31 Changed 4 years ago by
 Commit 880984aa7afbb49fff546e7163e30324a953df98 deleted
 Keywords sd71 added
comment:32 Changed 4 years ago by
 Keywords days71 added; sd71 removed
comment:33 Changed 4 years ago by
 Milestone changed from sage6.5 to sage7.2
I'm happy with this. Let me know when you want a review.