Opened 6 years ago

Closed 5 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 6 years ago by defeo

  • Description modified (diff)

comment:2 Changed 6 years ago by defeo

  • Description modified (diff)

comment:3 Changed 5 years ago by bruno

  • Branch set to u/bruno/t17190_error_conversion

comment:4 Changed 5 years ago by bruno

  • Authors set to Bruno Grenet
  • Commit set to c546e17684c29b2e5f108440205833b55e7622d5
  • Status changed from new to needs_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 5 years ago by bruno (previous) (diff)

comment:5 Changed 5 years ago by git

  • Commit changed from c546e17684c29b2e5f108440205833b55e7622d5 to fe6f95c764bbc69663ac6a68cc92723426ec40d2

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 follow-up: Changed 5 years ago by tscrim

  • Milestone changed from sage-6.4 to sage-6.9
  • Reviewers set to 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 5 years ago by git

  • Commit changed from fe6f95c764bbc69663ac6a68cc92723426ec40d2 to 6315d74ebf51bc82c26d6dafd389f52d895c8d78

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

6315d74#17190: Add missing blank line

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

  • Status changed from needs_review to positive_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 5 years ago by vbraun

  • Branch changed from u/bruno/t17190_error_conversion to 6315d74ebf51bc82c26d6dafd389f52d895c8d78
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.