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
small roots method for polynomials mod N (N composite) #2424
Comments
Attachment: coppersmith.patch.gz this patch addresses a concern raised by David via private communication |
comment:3
I can't comment on the mathematical correctness, but the patch looks good otherwise, and the examples are nice. Martin: if you can find someone else who knows about this stuff and is willing to look over the code, that would be good. Otherwise, if we don't have anyone else on the team with this expertise, let's just merge it in. |
comment:4
Doesn't this seem like there is an error in the code?
Seems obviously wrong, compared to
which seems right, but with a non-standard way of representing elements
It's also possible I'm completely overlooking something obvious. |
comment:5
Coppersmith's method certainly isn't designed for such very small
and
But there is at least one bug in my code, the return type is |
apply on top of coppersmith.patch (fixes issue discovered by wdj) |
Attachment: return-integers.patch.gz Attachment: coppersmith-X-bound.patch.gz this patch adapts the bound X such that the examples of David Joyner work |
comment:6
I've attached a new patch which fixes the issue reported by David Joyner above:
|
comment:7
Review by Bill Hart via private communication: I think the algorithm will fail if the content of f is not coprime to The polynomial should be made monic by multiplying by an inverse of Furthermore, failure to compute such an inverse of the leading I see that you need the parameter epsilon to be less than beta/7, but What happens if the user enters a value of beta which is negative or Currently if the user inputs a value of beta with beta <= deg(f)/8 Is LLL always guaranteed to return the vectors in order of length, The algorithm, as currently implemented, may return values which are The value of X should use the 1/2 factor as on page 34. The technique |
patch addresses review remarks by Bill Hart |
comment:8
Attachment: coppersmith-bhart-review.patch.gz Replying to @malb:
All this should be addressed by raising an error if the polynomial is not monic (including the content remark). We don't make the polynomial monic because this way the user has full control and can use the fact of being lucky and just having split N.
Updated accordingly.
The bounds are now enforced.
Yes.
Woops & updated accordingly.
Fixed. However, I was under the impression that Magma does something different here. In any case, the user can supply his/her own X. |
comment:9
Ummmm. The docstring says default value for epsilon is beta/8. But in the code it selects beta/7. Also, Bill seems to suggest above that epsilon needs to be less than beta/7 (or was it beta/8?) but this is not enforced anywhere in the code. |
comment:10
Replying to @sagetrac-dmharvey:
Fixed in
AFAIK, it doesn't need to be smaller than beta/7 but in my old code I assumed it was. The choice of epsilon is somewhat arbitrary ("choose an epsilon."). |
Attachment: small_roots_epsilon.patch.gz |
comment:11
Thumbs up from me. (Thanks Bill Hart for doing the hard work in this review.) |
comment:12
Merged all five patches in Sage 2.11.alpha0 - great work everybody. |
The attached patch implements ... well, lets just look at the docstring:
Why should this function be in Sage?
CC: @sagetrac-dmharvey
Component: number theory
Issue created by migration from https://trac.sagemath.org/ticket/2424
The text was updated successfully, but these errors were encountered: