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

Add positive root function of two algebraic numbers #57

Merged
merged 4 commits into from
Feb 28, 2022

Conversation

bobot
Copy link
Contributor

@bobot bobot commented Jan 13, 2022

I mainly wanted to understand how the library was built, so the approach is surely very naive. But I though it was sad that the algebraic numbers only provide operations that the rational provides. So this MR adds an implementation of the positive root of a positive algebraic number.

@disteph
Copy link
Contributor

disteph commented Jan 17, 2022

Looks good to me. Will see if @dddejan has comments.

Copy link
Collaborator

@nafur nafur left a comment

Choose a reason for hiding this comment

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

I'd suggest some improvements, mostly about comments/documentation.
I'm aware that my request go beyond the current average of libpoly, but libpoly should really improve in this respect... :-)

include/algebraic_number.h Outdated Show resolved Hide resolved
src/interval/arithmetic.h Outdated Show resolved Hide resolved
src/number/dyadic_rational.h Show resolved Hide resolved
src/number/algebraic_number.c Show resolved Hide resolved
Copy link
Member

@dddejan dddejan left a comment

Choose a reason for hiding this comment

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

Can you please add some tests.

The standard way is to add tests in Python. There is some work to add the new functionality to the Python API but this also helps make sure that the new API is useful.

You can follow what's already done in https://github.com/SRI-CSL/libpoly/tree/master/python to add the API. Then you can add the tests in https://github.com/SRI-CSL/libpoly/tree/master/test/python/tests.

include/algebraic_number.h Outdated Show resolved Hide resolved
src/upolynomial/upolynomial.c Outdated Show resolved Hide resolved
return the true rational when is_rational is true by adding the case
P(X)=den*X-num
@bobot
Copy link
Contributor Author

bobot commented Jan 25, 2022

Thank you for the review, improving documentation is always important. Three tests are added (other tests in the file algebraic_number.py are failing).

Copy link
Member

@dddejan dddejan left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@bobot
Copy link
Contributor Author

bobot commented Feb 4, 2022

What remains to be done for the PR to be merged ?

Playing with the new function I have seen that the degree of the polynomials can quickly grow even if we stay in the same simple extension (sorry I'm using words that I don't really understand), which should mean that any applications of field operations that use a uniq algebraic number of degree 2 and rationals should be representable with degree 2 .

For example : is represented with even if it is representable with as computed by .

Is it a known limitation?

@bobot
Copy link
Contributor Author

bobot commented Feb 7, 2022

One can play with which are computed by libpoly using a polynomial of degree even if their minimal polynome is of degree 2 (as any )

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

Successfully merging this pull request may close these issues.

4 participants