-
Notifications
You must be signed in to change notification settings - Fork 275
Port PML's geometric evaluation / interpolation integration for univariate polynomial #2449
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
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks for the work! Here is a first set of comments. I have not read through the interpolation part yet, but some comments should apply to there as well.
One main remark may be: before making further improvements, it seems like a good time to create an example file, check that this currently works, and then make sure nothing breaks with future changes. (Similarly, but this is less critical, create a profile file, measure the current performance, and check that nothing slows down with future changes.) If you need help in crafting these test/profile files let me know.
It seems that at some point the code could make use of nmod_vec_invert ( #2432 ).
Another thing to keep in mind: it also seems like many things may benefit from modular multiplication with precomputations. Yet this has the disadvantage of adding branches, because this cannot be used when the modulus has 64 bits... Anyway this may as well be considered later, once a first tested and profiled version is here.
|
|
||
| } nmod_geometric_progression_struct; | ||
|
|
||
| typedef nmod_geometric_progression_struct nmod_geometric_progression_t[1]; |
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 wondering whether these typedefs that should go here or in nmod_poly.h. Not sure about what is best.
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 followed as close as possible the structure for nmod_poly_evaluate_nmod_vec but the type for tree is just an opaque nn_ptr, I'm waiting on additional feedback.
| for (i = 0; i < d; i++) | ||
| { | ||
| vs[i] = 0; | ||
| } |
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.
If nmod_vec is included you may as well use _nmod_vec_zero.
| tmp = r; | ||
| for (i = 1; i < 2*d-1; i++) | ||
| { | ||
| nmod_poly_set_coeff_ui(G->f, i, nmod_mul(nmod_poly_get_coeff_ui(G->f, i-1), tmp, mod)); |
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.
Same remark as in other file: I think this could use direct coefficient access (G->f->coeffs[i] = ...), using once nmod_poly_set_length and nmod_poly_normalise after the loop. (Please ask if this comment is not clear enough.)
| tmp = nmod_mul(tmp, inv_q, mod); | ||
| } | ||
|
|
||
| inv_diff = _nmod_vec_init(d); |
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.
To be confirmed (todo for me): if I remember well, this is inverting a bunch of elements via the approach recently merged (#2432). If confirmed, we should see how to call that new _nmod_vec_invert.
| s = nmod_mul(s, inv_qk, mod); // prod 1/q^i | ||
| G->w[i] = nmod_mul(G->w[i-1], inv_diff[i], mod); // prod 1/(q^i-1) | ||
| tmp = nmod_mul(qq, G->w[i], mod); // prod q^i/(q^i-1) | ||
| nmod_poly_set_coeff_ui(G->g2, i, tmp); |
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.
We could see here as well about avoiding the use of set_coeff.
|
In Also, a remark: the vector |
Ah, yes, in fact several (most?) of my comments about avoiding
Yes, exactly. And there are other places in the geometric progression initialization where this may be useful as well (although less critical since this is done once for all). This is what I had in mind in my message above, when also suggesting to first having some tests/benchmark of the current code, and then seeing about incorporating this kind of speed-up. |
Co-authored-by: Vincent Neiger <[email protected]>
Co-authored-by: Vincent Neiger <[email protected]>
|
I will make adjustments to the PR for performance related issues once I have a working example, thanks a lot for your feedback ! |
For creating a basic test files for evaluation and interpolation (computes some evaluation, checks its correct by calling the function that supports any set of points), you could start from the files |
|
Oops, forget my last comment, I see work on this has already been started in recent commits. Something else to keep in mind: we will need to add some safety mechanism in case the provided |
This draft aims at porting @vneiger's code for geometric evaluation / interpolation (as mentioned in #1904 (comment)) with the end goal of having more efficient bivariate resultants.
nmod_poly_interpolate_nmod_vec_fast