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

raise ValueError instead of IndexError in .any_root() #37034

Conversation

yyyyx4
Copy link
Member

@yyyyx4 yyyyx4 commented Jan 8, 2024

The .any_root() method sometimes raises an IndexError instead of the usual ValueError when no root exists.

Example (Sage 10.2):

sage: R.<x> = Zmod(55)[]
sage: (x^2 + 1).any_root()
---------------------------------------------------------------------------
IndexError                                Traceback (most recent call last)
Cell In[2], line 1
----> 1 (x**Integer(2)+Integer(1)).any_root()

File /usr/lib/python3.11/site-packages/sage/rings/polynomial/polynomial_element.pyx:2315, in sage.rings.polynomial.polynomial_element.Polynomial.any_root (build/cythonized/sage/rings/polynomial/polynomial_element.c:34795)()
   2313                         return (self//h).any_root(ring, -degree, True)
   2314     else:
-> 2315         return self.roots(ring=ring, multiplicities=False)[0]
   2316
   2317 def __truediv__(left, right):

IndexError: list index out of range

With this patch, we check for the IndexError before it happens and raise a ValueError instead.

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall but some very minor things to address.

@yyyyx4
Copy link
Member Author

yyyyx4 commented Jan 9, 2024

Thanks, done.

Copy link

github-actions bot commented Jan 9, 2024

Documentation preview for this PR (built with commit d6660cd; changes) is ready! 🎉

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. LGTM.

vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 16, 2024
sagemathgh-37034: raise ValueError instead of IndexError in .any_root()
    
The `.any_root()` method sometimes raises an `IndexError` instead of the
usual `ValueError` when no root exists.

Example (Sage 10.2):
```sage
sage: R.<x> = Zmod(55)[]
sage: (x^2 + 1).any_root()
------------------------------------------------------------------------
---
IndexError                                Traceback (most recent call
last)
Cell In[2], line 1
----> 1 (x**Integer(2)+Integer(1)).any_root()

File /usr/lib/python3.11/site-
packages/sage/rings/polynomial/polynomial_element.pyx:2315, in
sage.rings.polynomial.polynomial_element.Polynomial.any_root
(build/cythonized/sage/rings/polynomial/polynomial_element.c:34795)()
   2313                         return (self//h).any_root(ring, -degree,
True)
   2314     else:
-> 2315         return self.roots(ring=ring, multiplicities=False)[0]
   2316
   2317 def __truediv__(left, right):

IndexError: list index out of range
```

With this patch, we check for the `IndexError` before it happens and
raise a `ValueError` instead.
    
URL: sagemath#37034
Reported by: Lorenz Panny
Reviewer(s): Travis Scrimshaw
vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 16, 2024
sagemathgh-37034: raise ValueError instead of IndexError in .any_root()
    
The `.any_root()` method sometimes raises an `IndexError` instead of the
usual `ValueError` when no root exists.

Example (Sage 10.2):
```sage
sage: R.<x> = Zmod(55)[]
sage: (x^2 + 1).any_root()
------------------------------------------------------------------------
---
IndexError                                Traceback (most recent call
last)
Cell In[2], line 1
----> 1 (x**Integer(2)+Integer(1)).any_root()

File /usr/lib/python3.11/site-
packages/sage/rings/polynomial/polynomial_element.pyx:2315, in
sage.rings.polynomial.polynomial_element.Polynomial.any_root
(build/cythonized/sage/rings/polynomial/polynomial_element.c:34795)()
   2313                         return (self//h).any_root(ring, -degree,
True)
   2314     else:
-> 2315         return self.roots(ring=ring, multiplicities=False)[0]
   2316
   2317 def __truediv__(left, right):

IndexError: list index out of range
```

With this patch, we check for the `IndexError` before it happens and
raise a `ValueError` instead.
    
URL: sagemath#37034
Reported by: Lorenz Panny
Reviewer(s): Travis Scrimshaw
vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 16, 2024
sagemathgh-37034: raise ValueError instead of IndexError in .any_root()
    
The `.any_root()` method sometimes raises an `IndexError` instead of the
usual `ValueError` when no root exists.

Example (Sage 10.2):
```sage
sage: R.<x> = Zmod(55)[]
sage: (x^2 + 1).any_root()
------------------------------------------------------------------------
---
IndexError                                Traceback (most recent call
last)
Cell In[2], line 1
----> 1 (x**Integer(2)+Integer(1)).any_root()

File /usr/lib/python3.11/site-
packages/sage/rings/polynomial/polynomial_element.pyx:2315, in
sage.rings.polynomial.polynomial_element.Polynomial.any_root
(build/cythonized/sage/rings/polynomial/polynomial_element.c:34795)()
   2313                         return (self//h).any_root(ring, -degree,
True)
   2314     else:
-> 2315         return self.roots(ring=ring, multiplicities=False)[0]
   2316
   2317 def __truediv__(left, right):

IndexError: list index out of range
```

With this patch, we check for the `IndexError` before it happens and
raise a `ValueError` instead.
    
URL: sagemath#37034
Reported by: Lorenz Panny
Reviewer(s): Travis Scrimshaw
@vbraun vbraun merged commit e5c6aab into sagemath:develop Jan 22, 2024
@yyyyx4 yyyyx4 deleted the public/raise_ValueError_consistently_in_any_root_method branch January 22, 2024 01:05
@mkoeppe mkoeppe added this to the sage-10.3 milestone Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants