-
Notifications
You must be signed in to change notification settings - Fork 27
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
Finish the flint_base
submodule, depreciate old roots and factor out more utils
#63
Finish the flint_base
submodule, depreciate old roots and factor out more utils
#63
Conversation
src/flint/flint_base/flint_base.pyx
Outdated
""" | ||
warn('This method is deprecated. Please instead use acb_poly(input_poly).roots()', DeprecationWarning) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warnings are useful only if we want to preserve existing behaviour while giving out the warning. This does not return the roots that would have previously been returned so existing behaviour is not preserved.
I think it is better just to raise an error here rather than give out a warning and implicity return None.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also if giving an error (or not preserving existing behaviour) then it does not really make sense to refer to this as "deprecated": rather it is "unsupported" or "disallowed" or something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree, I'll swap this for
raise NotImplementedError('This method has been deprecated. Please instead use acb_poly(input_poly).roots()')
Example:
Jack: python-flint % python3
Python 3.11.4 (main, Jul 25 2023, 17:07:07) [Clang 14.0.3 (clang-1403.0.22.14.1)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from flint import *
>>> acb_poly([1,2,3]).roots()
[[-0.333333333 +/- 7.53e-10] + [-0.471404521 +/- 7.03e-10]j, [-0.333333333 +/- 7.53e-10] + [0.471404521 +/- 7.03e-10]j]
>>> nmod_poly([1,2,3], 10).roots()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "src/flint/flint_base/flint_base.pyx", line 75, in flint.flint_base.flint_base.flint_poly.roots
raise NotImplementedError('This method has been deprecated. Please instead use acb_poly(input_poly).roots()')
NotImplementedError: This method has been deprecated. Please instead use acb_poly(input_poly).roots()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also we should add a CHANGELOG section at the bottom of the README with something like:
CHANGELOG
---------
0.5.0
- gh-63: The `roots` method of `fmpz_poly`, `fmpq_poly` and `nmod_poly` (others?) is no longer supported. Use `acb_roots(p).roots()` to get the old behaviour of returning the roots as `acb`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the simplest thing to do is simply say it's not supported and point to the acb_poly()
then...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fmpz_poly
still has roots, but they're complex roots, which is weird. fmpq_poly
asks for roots of the numerator, which I believe is of type fmpz_poly
, so the only ones currently broken I think are arb_poly
and nmod_poly
>>> acb_poly([1,2]).roots()
[-0.500000000000000]
>>> arb_poly([1,2]).roots()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "src/flint/flint_base/flint_base.pyx", line 75, in flint.flint_base.flint_base.flint_poly.roots
raise NotImplementedError('This method is no longer supported. To recover the complex roots first convert to acb_poly')
NotImplementedError: This method is no longer supported. To recover the complex roots first convert to acb_poly
>>> fmpz_poly([1,2]).roots()
[(-0.500000000000000, 1)]
>>> fmpq_poly([1,2]).roots()
[(-0.500000000000000, 1)]
>>> nmod_poly([1,2], 11).roots()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "src/flint/flint_base/flint_base.pyx", line 75, in flint.flint_base.flint_base.flint_poly.roots
raise NotImplementedError('This method is no longer supported. To recover the complex roots first convert to acb_poly')
NotImplementedError: This method is no longer supported. To recover the complex roots first convert to acb_poly
|
||
0.5.0 | ||
|
||
- gh-63: The `roots` method of `arb_poly`, and `nmod_poly` is no longer supported. Use `acb_roots(p).roots()` to get the old behaviour of returning the roots as `acb`. Note that the `roots` method of `fmpz_poly` and `fmpq_poly` currently returns the complex roots of the polynomial. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- gh-63: The `roots` method of `arb_poly`, and `nmod_poly` is no longer supported. Use `acb_roots(p).roots()` to get the old behaviour of returning the roots as `acb`. Note that the `roots` method of `fmpz_poly` and `fmpq_poly` currently returns the complex roots of the polynomial. | |
- gh-63: The `roots` method of `arb_poly`, and `nmod_poly` is no longer supported. Use `acb_roots(p).roots()` to get the old behaviour of returning the roots as `acb`. Note that the `roots` method of `fmpz_poly` and `fmpq_poly` previously returned the complex roots of the polynomial. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused here.
Why are the changes to arb_poly
being described differently to fmpz_poly
?
Is the roots method not being removed from all of them except acb_poly
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These classes still have a roots method. Although I've removed support from the very base class, the fmpz_poly has its own roots method which fmpq_poly inherits. One option is of course to delete the method here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see.
Okay then this is fine for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future I would like integer roots from fmpz and rational roots from fmpq, but this can wait until we link things up with Flint3 and the genetics I hope. For now this removal allowed the most simple refactoring to start.
Looks good. Thanks! |
Just a few additional things I wanted to do before finishing what I started in #61, this PR in part addresses Issue #62 by depreciating roots in the general case and asks the user to first convert to
acb_poly()