Opened 7 years ago
Closed 7 weeks ago
#17638 closed defect (fixed)
Division of polynomials produces errors when using local orderings
Reported by: | enriqueartal | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.7 |
Component: | commutative algebra | Keywords: | Singular, days94 |
Cc: | SimonKing, jakobkroeker, slelievre | Merged in: | |
Authors: | Mckenzie West | Reviewers: | Luis Felipe Tabera Alonso, Thierry Monteil |
Report Upstream: | N/A | Work issues: | |
Branch: | 8dd8ebe (Commits, GitHub, GitLab) | Commit: | 8dd8ebef54d67ec3f7aae96be0cfaa8392ce9278 |
Dependencies: | Stopgaps: |
Description (last modified by )
Consider two polynomials f,g in a ring define using a local ordering, e.g.,
R.<x,y>=PolynomialRing(QQ,order='neglex')
If the leading monomial of g is 1 then f/g will produces a wrong result, namely f divided by the independent coefficient of g. Note that in that case g.is_unit() yields True, since for Singular R is not Q[x,y] but the ordering localization (i.e., one can divide by the polynomials whose leading monomial is 1).
PS: It is my first ticket, I apologize for the mistakes.
Change History (35)
comment:1 Changed 7 years ago by
- Cc SimonKing added
- Component changed from algebraic geometry to commutative algebra
comment:2 Changed 7 years ago by
comment:3 Changed 7 years ago by
As far as I remember, in the definition of the division of two polynomials, it checks first if the denominator is a unit. If it is, it divides each monomial by the indepent coefficient of the polynomial. There may be two possible solutions. The simple one would be to change this behavior for the division. A more permanent one, which was discussed in this thread proposes to create a new class of localized rings.
comment:4 Changed 7 years ago by
Note that this is the code of "_div_" for multivariate polynomial rings:
cdef poly *p cdef MPolynomial_libsingular right = <MPolynomial_libsingular>right_ringelement cdef bint is_field = left._parent._base.is_field() if p_IsConstant(right._poly, right._parent_ring): if is_field: singular_polynomial_div_coeff(&p, left._poly, right._poly, right._parent_ring) return new_MP(left._parent, p) else: return left.change_ring(left.base_ring().fraction_field())/right_ringelement else: return (left._parent).fraction_field()(left,right_ringelement)
So, it is tested with the libsingular function !p_IsConstant whether the divisor is constant, hence, we can divide by the leading coefficient. I bet !p_IsConstant assumes a positive ordering and just tests whether the leading monomial is of degree zero.
comment:5 Changed 5 years ago by
- Cc jakobkroeker added
comment:6 Changed 5 years ago by
- Keywords Singular added
comment:7 follow-up: ↓ 8 Changed 4 years ago by
I checked the example again and it now works, is this fixed?
sage: R.<x,y>=PolynomialRing(QQ,order='neglex') sage: f = 1+y sage: g = 1+x sage: f/g (y + 1)/(x + 1)
comment:8 in reply to: ↑ 7 Changed 4 years ago by
Replying to mwest:
I checked the example again and it now works, is this fixed?
In any case, it deserves a dedicated doctest to prevent future regression.
comment:9 Changed 4 years ago by
- Branch set to u/mwest/division_of_polynomials_produces_errors_when_using_local_orderings
comment:10 Changed 4 years ago by
- Commit set to 989c4fc2850c173ea9034f22c7742c614840a770
- Keywords days94 added
New commits:
989c4fc | Added test to verify division works
|
comment:11 Changed 4 years ago by
- Status changed from new to needs_review
comment:12 Changed 4 years ago by
- Milestone changed from sage-6.5 to sage-8.3
This looks good to me. Note that the _div_ has not chaged.
Just one question, would it better if the test appears in src/sage/rings/polynomial/multi_polynomial_element.py? This is where the code in question is executed...
comment:13 Changed 4 years ago by
I added the test to the definition of div in src/sage/rings/polynomial/multi_polynomial_element.py are you thinking somewhere else?
comment:14 Changed 4 years ago by
yes, sorry, I was thinking of the method _div_ in src/sage/rings/polynomial/multi_polynomial_libsingular.pyx which is where the actual _div_ is performed for this ring.
comment:15 Changed 4 years ago by
- Commit changed from 989c4fc2850c173ea9034f22c7742c614840a770 to b958f3606d4518e240ac942d0502c7c9a3efb2c6
Branch pushed to git repo; I updated commit sha1. New commits:
b958f36 | properly formated ::
|
comment:16 Changed 4 years ago by
- Commit changed from b958f3606d4518e240ac942d0502c7c9a3efb2c6 to 703ad58668b598f6276bd4c4dbfe6c69e36ea08c
Branch pushed to git repo; I updated commit sha1. New commits:
703ad58 | Removed print function from test
|
comment:17 Changed 4 years ago by
- Commit changed from 703ad58668b598f6276bd4c4dbfe6c69e36ea08c to c2c1e9efd9b678739729fc755e70431e0cdd0f4b
Branch pushed to git repo; I updated commit sha1. New commits:
c2c1e9e | removed spacing for assignment within function
|
comment:18 Changed 4 years ago by
For reference, there is some documentation about formatting on the developer guide, see http://doc.sagemath.org/html/en/developer/coding_basics.html#python-code-style
In particular, you can add spacing between QQ,
and order="neglex"
, and perhaps between the +
(though the current code is already readable).
About the ::
, just to make things clearer than our previous discussion:
blah ::
it the secure way to write blah
followed by a block of code,
blah ::
is equivalent to the previous one.
However,
blah::
is equivalent to
blah: ::
which is therefore equivalent to:
blah: ::
[My personal preference goes to first and third, but it is up to you]
What was wrong in the commit 989c4fc2850c173ea9034f22c7742c614840a770 (comment 10) was
Ensure that :trac:`17638` is fixed.::
wich will print both a dot and a column, but i fear that now you removed both, while it might be good to keep a column.
comment:19 Changed 4 years ago by
- Commit changed from c2c1e9efd9b678739729fc755e70431e0cdd0f4b to 8dd8ebef54d67ec3f7aae96be0cfaa8392ce9278
Branch pushed to git repo; I updated commit sha1. New commits:
8dd8ebe | correcting white spacing
|
comment:20 Changed 4 years ago by
- Reviewers set to Luis Felipe Tabera Alonso, Thierry Monteil
- Status changed from needs_review to positive_review
comment:21 Changed 4 years ago by
It is all good with me, and with the newer location, we can see the test when we do:
sage: R.<x,y> = PolynomialRing(QQ, order='neglex') sage: f = 1 + y sage: f._div_??
comment:22 Changed 4 years ago by
- Milestone changed from sage-8.3 to sage-8.4
update milestone 8.3 -> 8.4
comment:23 follow-up: ↓ 30 Changed 4 years ago by
- Status changed from positive_review to needs_work
Merge conflict
comment:24 Changed 2 years ago by
- Cc slelievre added
- Description modified (diff)
- Milestone changed from sage-8.4 to sage-9.2
Discussed on sage-devel at:
comment:25 Changed 22 months ago by
- Milestone changed from sage-9.2 to sage-9.3
comment:26 Changed 17 months ago by
- Milestone changed from sage-9.3 to sage-9.4
Setting new milestone based on a cursory review of ticket status, priority, and last modification date.
comment:27 Changed 12 months ago by
- Milestone changed from sage-9.4 to sage-9.5
Setting a new milestone for this ticket based on a cursory review.
comment:28 Changed 7 months ago by
- Milestone changed from sage-9.5 to sage-9.6
comment:29 Changed 2 months ago by
- Milestone changed from sage-9.6 to sage-9.7
comment:30 in reply to: ↑ 23 Changed 2 months ago by
comment:31 Changed 2 months ago by
- Status changed from needs_work to positive_review
comment:32 Changed 7 weeks ago by
- Status changed from positive_review to needs_work
Merge failure on top of:
341d434082 Trac #33834: Fix map_reduce doctest
40dfa7a1a3 Trac #33828: Fix conda workflow
b90120d1cf Trac #33761: OpenSSL 3.0.3 security update
bdcb741d5b Trac #33700: Developer's guide: Expand on GitHub accounts and SSH keys
bf6aeb906d Updated SageMath version to 9.6
reviewer u'\u200bLuis Felipe Tabera Alonso, Thierry Monteil' does not look right
comment:33 Changed 7 weeks ago by
- Reviewers changed from Luis Felipe Tabera Alonso, Thierry Monteil to Luis Felipe Tabera Alonso, Thierry Monteil
- Status changed from needs_work to positive_review
comment:34 Changed 7 weeks ago by
removed the offending character
comment:35 Changed 7 weeks ago by
- Branch changed from u/mwest/division_of_polynomials_produces_errors_when_using_local_orderings to 8dd8ebef54d67ec3f7aae96be0cfaa8392ce9278
- Resolution set to fixed
- Status changed from positive_review to closed
Example: