Opened 7 years ago

Closed 4 years ago

Last modified 4 years ago

#13619 closed enhancement (fixed)

Enable polynomial content over padic fields

Reported by: saraedum Owned by: roed
Priority: major Milestone: sage-7.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 saraedum)

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)

trac_13619.patch (3.4 KB) - added by saraedum 7 years ago.

Download all attachments as: .zip

Change History (34)

comment:1 Changed 7 years ago by saraedum

  • Authors set to Julian Rueth

comment:2 Changed 7 years ago by saraedum

  • Description modified (diff)

comment:3 Changed 7 years ago by saraedum

  • Description modified (diff)

Changed 7 years ago by saraedum

comment:4 Changed 7 years ago by roed

I'm happy with this. Let me know when you want a review.

comment:5 Changed 7 years ago by tscrim

As a quick informal review, the OUTPUT: block is indented one too many times.

comment:6 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:7 Changed 6 years ago by niles

  • 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 niles

  • Commit set to 3ace36a14641ac23fc97d566905f21e562df5c4b

rebased and converted to git branch; changed indentation of OUTPUT: block, but no other changes


New commits:

3ace36aTrac #13619: Enable content for polynomials defined over Qp.

comment:9 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:10 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:11 Changed 5 years ago by saraedum

  • 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 saraedum

  • Keywords sd59 added
  • Status changed from new to needs_review

comment:13 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:14 follow-up: Changed 5 years ago by jdemeyer

  • 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:

e57bdb5Merge branch 'develop' into ticket/13619

comment:15 follow-up: Changed 5 years ago by jdemeyer

  • Milestone changed from sage-6.4 to sage-6.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 saraedum

Replying to jdemeyer:

Is there are reason you are only doing this for capped_relative_dense rings and not in the generic Polynomial_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 ; follow-ups: Changed 5 years ago by saraedum

Replying to jdemeyer:

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.

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 saraedum

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 ; follow-up: Changed 5 years ago by 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 sense
  • content_gen() returning an element, preferably a generator of the fractional ideal generated by the coefficients.

comment:20 in reply to: ↑ 17 ; follow-up: Changed 5 years ago by jdemeyer

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 saraedum

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 sense
  • content_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 saraedum

Replying to jdemeyer:

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.

Ok. You are right. I'll change it.

comment:23 Changed 5 years ago by git

  • Commit changed from e57bdb57f22542a6fe81a935be6ea8cc95d270e6 to 55e1b96cface91d3bd333c52ee868c668299e1f0

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

dc1dceeMerge branch 'develop' into t/13619/ticket/13619
55e1b96changed content of p-adic polynomials over Qp

comment:24 Changed 5 years ago by saraedum

  • Status changed from needs_work to needs_review

comment:25 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

See 14

comment:26 Changed 4 years ago by git

  • Commit changed from 55e1b96cface91d3bd333c52ee868c668299e1f0 to 355c1441df16255246733eb0808f43b420264b9c

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

355c144Merge branch 'develop' into t/13619/ticket/13619

comment:27 Changed 4 years ago by git

  • Commit changed from 355c1441df16255246733eb0808f43b420264b9c to 880984aa7afbb49fff546e7163e30324a953df98

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

880984aMove capped relative content() implementation to the generic p-adic polynomial class.

comment:28 Changed 4 years ago by saraedum

  • Status changed from needs_work to needs_review

comment:29 Changed 4 years ago by aly.deines

  • 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 vbraun

  • 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 aly.deines

  • Commit 880984aa7afbb49fff546e7163e30324a953df98 deleted
  • Keywords sd71 added

comment:32 Changed 4 years ago by aly.deines

  • Keywords days71 added; sd71 removed

comment:33 Changed 4 years ago by jdemeyer

  • Authors changed from Julian Rueth to Julian Rüth
  • Milestone changed from sage-6.5 to sage-7.2
Note: See TracTickets for help on using tickets.