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

Fixed divrem bugs, added pseudo divrem #72

Merged
merged 15 commits into from
Nov 3, 2023
Merged

Conversation

Ovascos
Copy link
Contributor

@Ovascos Ovascos commented Nov 1, 2023

  • added assertion to warn about incorrect dereference in function coefficient_div when dividing a constant by a non-constant polynomial
  • fixed incorrect D output parameter in coefficient_divrem when calling with two constant polynomials
  • use dense pseudo remainder in polynomial_reduce. Previously it expected lc divisibility rendering it just another divrem function
  • added lp_polynomial_pdivrem and lp_polynomial_spdivrem for (sparse) pseudo division with remainder and added python bindings for it
  • removed unimplemented function prototype lp_polynomial_context from polynomial.h
  • update trace.h to avoid some potential preprocessor replacement pitfalls (used do {} while(0) for multiple lines, use ((void)0) instead of empty command in release builds)
  • Fixed algebraic number test, added return value for check to indicate failed tests
  • fixed some typos

@@ -669,7 +719,7 @@ void lp_polynomial_divrem(lp_polynomial_t* D, lp_polynomial_t* R, const lp_polyn
lp_polynomial_set_context(D, A1->ctx);
lp_polynomial_set_context(R, A1->ctx);

coefficient_divrem(D->ctx, &R->data, &R->data, &A1->data, &A2->data);
coefficient_divrem(D->ctx, &D->data, &R->data, &A1->data, &A2->data);
Copy link
Member

Choose a reason for hiding this comment

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

Nice catch

@@ -220,7 +220,7 @@ void lp_polynomial_sub_mul(lp_polynomial_t* S, const lp_polynomial_t* A1, const
*
* and
*
* P in Z[y]
* P in Z[x]
Copy link
Member

Choose a reason for hiding this comment

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

This conflicts with the comment of the coefficient_reduce method. Are you sure about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot to change this at coefficient_reduce as well.

@@ -159,4 +159,4 @@ def check_comparison(a1, a2, cmp, result, expected):
p = functools.reduce(lambda x, y: x*y, sample, one)
random.shuffle(sample)
p = functools.reduce(lambda x, y: x/y, sample, p)
polypy_test.check(p == 1)
polypy_test.check(p == one)
Copy link
Member

Choose a reason for hiding this comment

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

nice catch

Copy link
Member

@ahmed-irfan ahmed-irfan left a comment

Choose a reason for hiding this comment

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

LGTM!

Thanks

@ahmed-irfan ahmed-irfan merged commit 862d7cb into SRI-CSL:master Nov 3, 2023
8 checks passed
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.

2 participants