Elliptic curve

[abhinav-anand-addepar]

revives #2449

• ntheory
  • implemented elliptic curve

[sympy-bot]

🟠

Hi, I am the SymPy bot (v157). I've noticed that some of your commits add or delete files. Since this is sometimes done unintentionally, I wanted to alert you about it.

This is an experimental feature of SymPy Bot. If you have any feedback on it, please comment at sympy/sympy-bot#75.

The following commits add new files:

• 5b5e17b:
  • sympy/ntheory/ec.py
• 51c044b:
  • sympy/ntheory/tests/test_ec.py

If these files were added/deleted on purpose, you can ignore this message.

[sympy-bot]

✅

Hi, I am the SymPy bot (v157). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

• ntheory
  • implemented elliptic curve (#18682 by @abhinav28071999 and @shikil)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.6.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

revives #2449 

<!-- BEGIN RELEASE NOTES -->
* ntheory
  * implemented  elliptic curve 
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

[abhinav-anand-addepar]

Can someone help me to resolve this please.

[jksuom]

It looks like you should merge current master (https://github.com/sympy/sympy/wiki/Development-workflow#how-to-merge) before adding new commits. The last one should be removed first (git reset HEAD^). (If that does not work, you can remove the branch: git branch -D elliptio-curve and fetch it again.)

[abhinav-anand-addepar]

now its okay, i guess.

[sylee957]

Are you going to make the object an instance of Basic, or make it some dedicated solver that should not be Basic?

If this is not made Basic, I’d not ship with the global namespace, or I'd name it more trivially like EllipticCurveSolver because I'd expect naming issues like #18672 can arise in the future, if someone tries to introduce a proper 'Basic' elliptic curve later.

If it is made Basic, I'd drop much of the problematic domain stuff like QQ but fix it to use Rational or Integer with modulus.

[abhinav-anand-addepar]

@jksuom I read about elliptic curves and found that there is a lot of difference in algorithms when the domain is QQ or ,ZZ or is finite. So i suggest making difference class for elliptic curves over finite fields and rational fields.

[jksuom]

The domain should be a field so ZZ need not be considered. The same code will work over almost all fields, different classes are necessary only in characteristics 2 and 3. Those are not important for applications in number theory so I would not include them now to save time. (Classes for them can be added later if someone is interested.)

[abhinav-anand-addepar]

i have corrected the points function. I think that most of the things implemented was wrong. I will correct those.

[sylee957]

SymPy's notation of finite field is wrong, and it may actually be called a 'field' if the order is prime
#13886

[jksuom]

The first task is to make doctests pass. I have added some comments to that end.

[abhinav-anand-addepar]

@jksuom I have a doubt. When adding point is done, when domain is QQ we have to perform a 1/QQ(num) and when finite field is used then we will need mod_inverse(FF(modulus)(num), modulus). But due to #18703 and #18706 , how we will perform.

[jksuom]

Division 1/a should work both in QQ and in a finite field.

[abhinav-anand-addepar]

e = EllipticCurve(2,3 , modulus = 97 )
p = e(3,6)
p+p
it returns (-17, 10) which is correct but it should be in the domain.
I tried dom(x3), dom(y3) before returning point addition, but didn't work.

[abhinav-anand-addepar]

@jksuom is there any trick to get through #18703 because the errors are probably due to that.

[jksuom]

The arguments dom=QQ should be integers. That is not true on line 316 in general.

[jksuom]

This will probably be the same as return self.__add__(-other).

[jksuom]

I have a few minor comments but otherwise I think that this is ready be merged. Only the name should be changed. ec.py is not informative, I would suggest elliptic_curves.py (git mv ...).

[abhinav-anand-addepar]

@jksuom Done.

[abhinav-anand-addepar]

how to remove this circular import error?

[abhinav-anand-addepar]

@jksuom I am not able to push the changes it is giving this error.

[abhinav-anand-addepar]

It got push, don't know why it was showing error earlier.

[abhinav-anand-addepar]

when i added elliptic curve in ntheory\__init__.py it is giving circular import error.

[jksuom]

I wonder how that did happen. Only the module name was changed.

[jksuom]

It looks like GitHub had some issues: https://twitter.com/githubstatus?lang=en

It is probably not necessary to import elliptic curves from __init__.py at this point.

[jksuom]

It looks like GitHub had some issues: https://twitter.com/githubstatus?lang=en

It is probably not necessary to import elliptic curves from __init__.py at this point.

[abhinav-anand-addepar]

should i squash all the commits.

[jksuom]

We should preserve the commits of the original author, or at least some of them. Can you squash selected commits?

[abhinav-anand-addepar]

okay.

[abhinav-anand-addepar]

Done. Is this okay?

[jksuom]

I'll look more closely tomorrow but I think it is ok.

[jksuom]

Thanks, this should be a good basis for computations with elliptic curves. It can be extended later if desired. The rank method may be too hard to implement (I don't know of any algorithm) but it should be harmless to leave the stub alone.

I have removed WIP from the title.

[abhinav-anand-addepar]

@jksuom Hey, I have made the proposal for quadratic sieve and Elliptic curve factorization. Here is the link. I have posted this on mailing list but there are too many posts coming lately so i though that this might skip. Please go through it and suggest any changes.
Thank you