Add rational parametrization for algebraic curves#106
Add rational parametrization for algebraic curves#106mohabsafey merged 43 commits intoalgebraic-solving:mainfrom
Conversation
|
Many thanks Rémi for this PR. I started to review your code ; I will go through in the coming days ; I may be slow but there are already two comments with suggestions of modifications |
src/algorithms/param-curve.jl
Outdated
| R = parent(first(F)) | ||
| N = nvars(R) | ||
| if check_gen let | ||
| local INEW = Ideal(change_base_ring.(Ref(GF(65521)), vcat(F, gens(R)[N-1]-rand(ZZ,-100:100)))) |
There was a problem hiding this comment.
Line 69: same here, it is a bit dangerous to do these tests again over the same (small) prime field GF(65521); some randomisation should be introduced
There was a problem hiding this comment.
Also done in e3837be.
Do you think we should take a different p than above in Itest?
There was a problem hiding this comment.
Yes, I think you should take here a different prime to strengthen the tests
|
@mohabsafey @rprebet Is this ready to be merged from your point of view? If so I would like to add it to the v0.10.0 release. |
|
There is still one change to perform in the code (some missing genericity test). Rémi is working on this. Can we wait for a couple of days before having the next release ? |
|
No problem, thanks for working on it. |
|
I'm done |
3 participants
I chose the terminology and data structure according to the
rational_parametrizationfunction as it can be seen as a one-dimensional version of it. For this reason I included it in the "solvers" documentation page.However the source code in a separate file
param-curve.jlas it is not just an interface to msolve.