Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong basic interval arithmetic in PolynomialRing #13760

Closed
sagetrac-gmoroz mannequin opened this issue Nov 26, 2012 · 31 comments
Closed

Wrong basic interval arithmetic in PolynomialRing #13760

sagetrac-gmoroz mannequin opened this issue Nov 26, 2012 · 31 comments

Comments

@sagetrac-gmoroz
Copy link
Mannequin

sagetrac-gmoroz mannequin commented Nov 26, 2012

Some multiplications in multivariate polynomial rings over RIF or CIF are wrong:

sage: R.<x,y> = PolynomialRing(RIF,2)
sage: RIF(-2,1)*x
0

More tests

sage: R.<x,y> = PolynomialRing(RIF,2)
sage: RIF(-2,1)          # OK
0.?e1
sage: RIF(-2,1)*x        # Wrong
0
sage: RIF(-2,1)*x == 0   # Wrong
True
sage: cmp(RIF(-2,1)*x,0) # Wrong
0
sage: RIF(2,5)*x         # OK
1.?e1*x
sage: RIF(2,5)*x == 0    # OK
False

Code digging

The problem comes from the coercion:

sage: R(RIF(-2,1))
0
sage: R(RIF(-2,1)) == 0
True

This in turn comes from the creation (in __init__ of class MPolynomial_polydict in sage.rings.polynomial.multi_polynomial_element) of a PolyDict object (defined in sage.rings.polynomial.polydict) with the option remove_zero == True.

from sage.rings.polynomial.polydict import PolyDict
sage: PolyDict({(0,0):1}, remove_zero=True) # OK
PolyDict with representation {(0, 0): 1}
sage: PolyDict({(0,0):0}, remove_zero=True) # OK
PolyDict with representation {}
sage: PolyDict({(0,0):RIF(-1,1)}, remove_zero=True) # Wrong
PolyDict with representation {}

To check if x is different from 0, PolyDict uses the test x != 0, which actually checks for disjointness in interval field:

sage: RIF(-2,1) != 0
False

Possible correction

This bug might be corrected by replacing in PolyDict the tests x != zero by one of:

  • cmp(x,zero) != 0
  • not x.is_zero()
  • not x == zero

Apply:

CC: @burcin @mwhansen @malb @zimmermann6

Component: basic arithmetic

Keywords: interval, polynomial, multivariate

Author: Guillaume Moroz

Reviewer: Paul Zimmermann

Merged: sage-5.6.beta1

Issue created by migration from https://trac.sagemath.org/ticket/13760

@sagetrac-gmoroz sagetrac-gmoroz mannequin added this to the sage-5.5 milestone Nov 26, 2012
@sagetrac-gmoroz sagetrac-gmoroz mannequin assigned aghitza Nov 26, 2012
@sagetrac-gmoroz

This comment has been minimized.

@sagetrac-gmoroz
Copy link
Mannequin Author

sagetrac-gmoroz mannequin commented Nov 26, 2012

A patch changing x != zero by not x == zero.

@sagetrac-gmoroz

This comment has been minimized.

@sagetrac-gmoroz
Copy link
Mannequin Author

sagetrac-gmoroz mannequin commented Nov 26, 2012

comment:2

Attachment: trac_13760_interval_polynomial_fix.patch.gz

@sagetrac-gmoroz
Copy link
Mannequin Author

sagetrac-gmoroz mannequin commented Nov 26, 2012

Author: gmoroz

@zimmermann6
Copy link

comment:5

Guillaume, a non-regression test is missing (also for #7712).

Paul

@zimmermann6
Copy link

Work Issues: add non-regression test

@zimmermann6
Copy link

comment:7

all tests pass on top of 5.4.1, just waiting for a non-regression test.

Paul

@sagetrac-gmoroz
Copy link
Mannequin Author

sagetrac-gmoroz mannequin commented Dec 13, 2012

add regression tests (to be applied after the fix patch)

@zimmermann6
Copy link

comment:8

Attachment: trac_13760_regression_tests.patch.gz

Guillaume, why didn't you add as regression tests the examples in the description of #7712 and this ticket?

Paul

@zimmermann6
Copy link

comment:9

all tests pass with the regression tests.

Paul

@sagetrac-gmoroz
Copy link
Mannequin Author

sagetrac-gmoroz mannequin commented Dec 14, 2012

comment:10

I attached the patch with regression tests with user level examples on #7712 (this choice was quite random).

@sagetrac-gmoroz
Copy link
Mannequin Author

sagetrac-gmoroz mannequin commented Dec 14, 2012

Includes regression tests for #7712 and #13760 (same patch as in #7712, moved here for convenience)

@zimmermann6
Copy link

comment:12

Attachment: trac_7712_regression_tests_for_13760_fix.patch.gz

@zimmermann6
Copy link

Changed author from gmoroz to Guillaume Moroz

@zimmermann6
Copy link

Reviewer: Paul Zimmermann

@zimmermann6
Copy link

Changed work issues from add non-regression test to none

@jdemeyer
Copy link

comment:13

Please indicate which patch(es) need to be applied.

@jdemeyer jdemeyer modified the milestones: sage-5.5, sage-5.6 Dec 15, 2012
@zimmermann6

This comment has been minimized.

@zimmermann6
Copy link

comment:15

Replying to @jdemeyer:

Please indicate which patch(es) need to be applied.

Done.
Paul

@tscrim
Copy link
Collaborator

tscrim commented Dec 15, 2012

comment:16

One very minor documentation thing, there should be a blankline after any double colon :: (see line 514 in the attachment: trac_13760_regression_tests.patch). Thanks.

@sagetrac-gmoroz
Copy link
Mannequin Author

sagetrac-gmoroz mannequin commented Dec 19, 2012

comment:17

Replying to @tscrim:

One very minor documentation thing, there should be a blankline after any double colon :: (see line 514 in the attachment: trac_13760_regression_tests.patch). Thanks.

It seems that I cannot attach a new patch for that. Is it because of the positive review status?

@tscrim
Copy link
Collaborator

tscrim commented Dec 19, 2012

Work Issues: docstrings

@tscrim
Copy link
Collaborator

tscrim commented Dec 19, 2012

comment:18

Replying to @sagetrac-gmoroz:

It seems that I cannot attach a new patch for that. Is it because of the positive review status?

Strange...I don't think so, but I'll set this to needs work and try it then.

@sagetrac-gmoroz
Copy link
Mannequin Author

sagetrac-gmoroz mannequin commented Dec 19, 2012

Attachment: trac_13760_documentation_fix.patch.gz

@sagetrac-gmoroz

This comment has been minimized.

@sagetrac-gmoroz
Copy link
Mannequin Author

sagetrac-gmoroz mannequin commented Dec 19, 2012

comment:20

Replying to @tscrim:

Strange...I don't think so, but I'll set this to needs work and try it then.

My bad, it appears that my file didn't have permissions to be read.

@tscrim

This comment has been minimized.

@tscrim
Copy link
Collaborator

tscrim commented Dec 19, 2012

comment:21

Thanks for fixing this.

@jdemeyer
Copy link

Changed work issues from docstrings to none

@jdemeyer
Copy link

Merged: sage-5.6.beta1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants