Opened 7 years ago

Last modified 2 years ago

#13697 needs_info enhancement

Improvements and corrections to QuotientRingElement

Reported by: Bouillaguet Owned by: malb
Priority: minor Milestone: sage-6.4
Component: commutative algebra Keywords: sd86.5
Cc: Merged in:
Authors: Charles Bouillaguet Reviewers:
Report Upstream: N/A Work issues:
Branch: u/saraedum/ticket/13697 (Commits) Commit: 6276d0b8134008954f31b66c3055ae946078c657
Dependencies: #13670,#13671,#13675,#13714 Stopgaps:

Description (last modified by saraedum)

The class QuotientRingElement, which implements the operations an element of the quotient R/I of a ring R by an ideal I, suffers from several problems and limitations. Most of these were uncovered while working on #13670 and #13675.

  • While QuotientRingElement aims to be generic, it contains code dedicated to the case where R is a multivariate polynomial ring. In particular, the implementation of division first checks if R supports the computation of Groebner bases. This is not the proper way to go ; a special class should be created, following the approach taken for univariate polynomial quotient rings (PolynomialQuotientRing, PolynomialQuotientRingElement). We should create MPolynomialQuotientRing and MPolynomialQuotientRingElement, and host multivariate-polynomial-specific code here.
  • The is_unit() function does almost nothing (it checks if its argument is a unit in R). In the case of (multivariate) polynomial rings, an actual test can be implemented.
  • The class lacks an is_regular() methods (that detects zero divisors).
  • In the case of (multivariate) polynomial rings, is_regular() can be implemented.
  • The interest of is_regular() is that division by x should only be allowed if x is regular.
  • The present implementation of division has problems. It contains multivariate-polynomial-specific code, which is bad. Furthermore, it allows division by zero-divisors, even tough the result is not defined :
    sage: R.<x,y> = QQ[]
    sage: S = R.quotient_ring(R.ideal(x^2, y))
    sage: S(2*x)/S(x)
    S(2)
    sage: S(2) * S(x) == S(2*x)  # indeed, division works correctly....
    True
    sage: S(2+x) * S(x) == S(2*x) # but several "quotients" are possible, because ``S(x)`` is a zero-divisor
    

In contrast, univariate polynomial rings behave more rigorously:

sage: P.<x> = QQ[]
sage: S = P.quotient_ring(x^2)
sage: S(2*x)/S(x)
ZeroDivisionError: element xbar of quotient polynomial ring not invertible
  • This raises the question of how we want division to proceed:
    • ignore the problem? (current status, no overhead)
    • test for regularity before dividing (mathematically better, may be much slower)
  • Clarifying all this would then open the possibility to have, for example, special code to deal with ideals given by a regular chain instead of a Groebner basis

Apply

  1. trac_13697.patch

Attachments (2)

improve_quotient_ring.patch (30.3 KB) - added by Bouillaguet 7 years ago.
trac_13697.patch (17.1 KB) - added by saraedum 6 years ago.
rebase of Bouillaguet's patch

Download all attachments as: .zip

Change History (19)

comment:1 Changed 7 years ago by Bouillaguet

  • Dependencies changed from #13670,#13671,#13675 to #13670,#13671,#13675,#13714

Changed 7 years ago by Bouillaguet

comment:2 Changed 7 years ago by Bouillaguet

  • Status changed from new to needs_review

comment:3 Changed 6 years ago by saraedum

  • Authors set to Charles Bouillaguet

comment:4 Changed 6 years ago by saraedum

I'll rebase this patch and try to review it.

Changed 6 years ago by saraedum

rebase of Bouillaguet's patch

comment:5 Changed 6 years ago by saraedum

  • Description modified (diff)

apply trac_13697.patch

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 saraedum

  • Branch set to u/saraedum/ticket/13697
  • Created changed from 11/10/12 16:44:59 to 11/10/12 16:44:59
  • Modified changed from 08/13/13 15:35:53 to 08/13/13 15:35:53

comment:8 Changed 6 years ago by saraedum

  • Description modified (diff)
  • Milestone changed from sage-5.12 to sage-6.0

comment:9 Changed 6 years ago by git

  • Commit set to 756831a00ba9a0a1624e9b516621d5420a7cf2f7

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

[changeset:756831a]added a doctest for QuotientRingElement?.is_unit()
[changeset:1967e3b]removed trailing whitespace and semicolons from docstrings

comment:10 Changed 6 years ago by saraedum

  • Modified changed from 09/12/13 15:07:35 to 09/12/13 15:07:35
  • Status changed from needs_review to needs_info

Your patch does not address all the questions in the Ticket summary. How do you want to proceed with this? Should we split this and move the issues to new tickets?

comment:11 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.0 to sage-6.1

comment:12 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:13 Changed 5 years ago by pbruin

I ran into the following:

sage: R.<x,y>=QQ[]
sage: Q.<xx,yy>=R.quotient(x^2-y^3)
sage: xx/yy
...
ArithmeticError: Division failed. The numerator is not a multiple of the denominator.

It would be nice if in this situation, the quotient ring could check if it is a domain, and if so, return xx/yy as an element of the fraction field.

comment:14 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:15 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:16 Changed 2 years ago by git

  • Commit changed from 756831a00ba9a0a1624e9b516621d5420a7cf2f7 to 6276d0b8134008954f31b66c3055ae946078c657

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

6276d0bMerge branch 'develop' into t/13697/ticket/13697

comment:17 Changed 2 years ago by saraedum

  • Keywords sd86.5 added
Note: See TracTickets for help on using tickets.