Opened 8 years ago

Closed 7 years ago

#17190 closed defect (fixed)

Error in conversion from RR['x,y'] to RR['x']

Reported by: defeo Owned by:
Priority: major Milestone: sage-6.9
Component: commutative algebra Keywords: polynomials RR
Cc: Merged in:
Authors: Bruno Grenet Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 6315d74 (Commits, GitHub, GitLab) Commit: 6315d74ebf51bc82c26d6dafd389f52d895c8d78
Dependencies: Stopgaps:

Status badges

Description (last modified by defeo)

Conversion of univariate polynomials from the bivariate ring RR['x,y'] fails:

sage: RR['x'](RR['x,y'].0)

----
ValueError                                Traceback (most recent call last)
...
ValueError: max() arg is an empty sequence

This does not seem to affect other rings:

sage: QQ['x'](QQ['x,y'].0)
x
sage: CC['x'](CC['x,y'].0)
x
sage: RDF['x'](RDF['x,y'].0)
x

Change History (9)

comment:1 Changed 8 years ago by defeo

Description: modified (diff)

comment:2 Changed 8 years ago by defeo

Description: modified (diff)

comment:3 Changed 7 years ago by bruno

Branch: u/bruno/t17190_error_conversion

comment:4 Changed 7 years ago by bruno

Authors: Bruno Grenet
Commit: c546e17684c29b2e5f108440205833b55e7622d5
Status: newneeds_review

[EDIT] As one can notice, I messed up the history with some completely unrelated commits. I am trying to undo this... The relevant code is the latest commit only! [/EDIT] Should be OK now!

The problem was not in conversion, but rather in initialization of univariate polynomials over RR, with an empty dictionary. Before:

sage: RR['x']({})
Traceback (most recent call last):
...
ValueError: max() arg is an empty sequence

After:

sage: RR['x']({})
0

As a result, we also have:

sage: RR['x'](RR['x,y'].0)
x

Details

In the initialization of a polynomial over RR, if the "data" is given as a dictionary, this dictionary is converted to a list. Before the patch, there was an attempt to compute max(x.keys()) where x is the dictionary. If x is empty, this yields an error. To correct the error, I could have add a test. Yet, a function is already specifically defined in the class Polynomial to perform this conversion dict -> list. The new code just uses this function.

Note that I corrected some slight formatting issues in the docstring in other (unrelated) parts of the same file src/sage/rings/polynomials/polynomial_real_mpfr_dense.pyx.


New commits: (Note: Irrelevant commits. The only correct commit is in comment:5 below)

98b7ca1Rebased on 6.9beta1
6d0da24One last zero_element removed + correct one doctest
dbfb518Remove useless colons
331c713#19171: New method divides
7527104#19172: Rewrite valuation for dense polynomials
22d9c67#19172: valuation for sparse polynomials
c546e17#17190: Authorize empty dictionary in initialization of polynomial over RR
Last edited 7 years ago by bruno (previous) (diff)

comment:5 Changed 7 years ago by git

Commit: c546e17684c29b2e5f108440205833b55e7622d5fe6f95c764bbc69663ac6a68cc92723426ec40d2

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

fe6f95c#17190: Authorize empty dictionary in initialization of polynomial over RR

comment:6 Changed 7 years ago by tscrim

Milestone: sage-6.4sage-6.9
Reviewers: Travis Scrimshaw

LGTM modulo adding a blank line here:

         Check that :trac:`17190` is fixed::
+
             sage: RR['x']({})
             0

Once you make this change, you can set a positive review on my behalf. (Also, could you edit you post above to show the result is 0?)

comment:7 Changed 7 years ago by git

Commit: fe6f95c764bbc69663ac6a68cc92723426ec40d26315d74ebf51bc82c26d6dafd389f52d895c8d78

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

6315d74#17190: Add missing blank line

comment:8 in reply to:  6 Changed 7 years ago by bruno

Status: needs_reviewpositive_review

Replying to tscrim:

LGTM modulo adding a blank line here:

         Check that :trac:`17190` is fixed::
+
             sage: RR['x']({})
             0

Once you make this change, you can set a positive review on my behalf. (Also, could you edit you post above to show the result is 0?)

Thanks for the quick review! (I edited my post to make it correct...)

comment:9 Changed 7 years ago by vbraun

Branch: u/bruno/t17190_error_conversion6315d74ebf51bc82c26d6dafd389f52d895c8d78
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.