Opened 7 months ago

Closed 7 months ago

#15855 closed enhancement (fixed)

Clean up weierstrass_p

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-6.2
Component: elliptic curves Keywords:
Cc: cremona, pbruin Merged in:
Authors: Jeroen Demeyer Reviewers: Peter Bruin
Report Upstream: N/A Work issues:
Branch: 666714e (Commits) Commit: 666714eb35225661954ce037892f40d866cc04c3
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

Fix the precision bound for the quadratic algorithm and clean up the code.

Change History (19)

comment:1 Changed 7 months ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 7 months ago by jdemeyer

  • Branch set to u/jdemeyer/ticket/15855
  • Created changed from 02/24/14 04:39:24 to 02/24/14 04:39:24
  • Modified changed from 02/24/14 05:29:43 to 02/24/14 05:29:43

comment:3 Changed 7 months ago by git

  • Commit set to 1e692657b19d5ae80cd420e6a0c801a66cec6644

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

1e69265Clean up weierstrass_p function

comment:4 Changed 7 months ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Status changed from new to needs_review

New commits:

1e69265Clean up weierstrass_p function

comment:5 Changed 7 months ago by pbruin

An argument to be substituted for %s is missing in the following line:

raise ValueError("for computing the Weierstrass p-function via pari, the characteristic (%s) of the underlying field must be zero")

comment:6 Changed 7 months ago by git

  • Commit changed from 1e692657b19d5ae80cd420e6a0c801a66cec6644 to 89726c59cdf83f3cee87e56806a3a86d8938df2c

Branch pushed to git repo; I updated commit sha1. New commits:

89726c5Fix exception formatting

comment:7 Changed 7 months ago by pbruin

  • Status changed from needs_review to positive_review

comment:8 Changed 7 months ago by pbruin

In the meantime, Karim Belabas fixed the elliptic functions in PARI for base fields of positive characteristic. Maybe we should just change the test for the characteristic in the case algorithm='pari' accordingly?

comment:9 Changed 7 months ago by pbruin

  • Description modified (diff)
  • Status changed from positive_review to needs_info

I did some further cleaning up on a new branch u/pbruin/15855-weierstrass_p_cleanup. The test for p >= prec + 3 can also be done at the beginning, since p really arise in the denominators of the series, we are not just dividing by it as an intermediate step of the algorithm. With this patch, we don't insist on characteristic 0 when algorithm='pari', but neither do we use PARI by default. If you agree with the changes in this branch and it still good as a dependency of #15767, you can put it in the branch field.

We probably have to check if everything still works after the latest PARI fix, hence needs_info.

comment:10 follow-up: Changed 7 months ago by jdemeyer

Alternatively, repurpose this ticket as a cleanup ticket and leave the algorithm choice as it was.

comment:11 in reply to: ↑ 10 ; follow-up: Changed 7 months ago by jdemeyer

Replying to jdemeyer:

Alternatively, repurpose this ticket as a cleanup ticket and leave the algorithm choice as it was.

Peter, let me know if this sounds like a good idea, then I will implement it.

comment:12 in reply to: ↑ 11 Changed 7 months ago by pbruin

Replying to jdemeyer:

Replying to jdemeyer:

Alternatively, repurpose this ticket as a cleanup ticket and leave the algorithm choice as it was.

Peter, let me know if this sounds like a good idea, then I will implement it.

Yes, if PARI's ellwp() works as intended again, I think that is the best plan.

comment:13 Changed 7 months ago by jdemeyer

  • Description modified (diff)
  • Summary changed from Disable PARI algorithm for weierstrass_p in positive characteristic to Clean up weierstrass_p

comment:14 Changed 7 months ago by git

  • Commit changed from 89726c59cdf83f3cee87e56806a3a86d8938df2c to 0e4fff510181e974717899f802108be8877a9346

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

0e4fff5Clean up weierstrass_p function

comment:15 Changed 7 months ago by jdemeyer

  • Status changed from needs_info to needs_review

comment:16 follow-up: Changed 7 months ago by pbruin

  • Reviewers set to Peter Bruin

The last part of the new changelog entry in ell_wp.py is no longer accurate. Besides that, positive review.

(Maybe the additional change proposed in comment:9 isn't the best one; perhaps there are elliptic curves where more coefficients of the series are well defined, in which case a NotImplementedError is better than an algorithm-independent ValueError.)

comment:17 Changed 7 months ago by git

  • Commit changed from 0e4fff510181e974717899f802108be8877a9346 to 666714eb35225661954ce037892f40d866cc04c3

Branch pushed to git repo; I updated commit sha1. New commits:

666714eFix changelog entry for ell_wp

comment:18 in reply to: ↑ 16 Changed 7 months ago by jdemeyer

  • Status changed from needs_review to positive_review

Replying to pbruin:

The last part of the new changelog entry in ell_wp.py is no longer accurate. Besides that, positive review.

Done.

(Maybe the additional change proposed in comment:9 isn't the best one; perhaps there are elliptic curves where more coefficients of the series are well defined, in which case a NotImplementedError is better than an algorithm-independent ValueError.)

I have no idea, I don't know the mathematics well enough.

comment:19 Changed 7 months ago by vbraun

  • Branch changed from u/jdemeyer/ticket/15855 to 666714eb35225661954ce037892f40d866cc04c3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.