Opened 5 years ago

Closed 5 years ago

#19382 closed enhancement (fixed)

move elliptic_curve_congruence graph from graphs to elliptic_curves

Reported by: cremona Owned by:
Priority: major Milestone: sage-6.10
Component: elliptic curves Keywords: elliptic curve congruence graph
Cc: ncohen Merged in:
Authors: John Cremona Reviewers: Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: 5607dac (Commits) Commit: 5607dac76b7409d68a72d501de00339b3bdbc6d2
Dependencies: Stopgaps:

Description

From sage-devel:

It is possible to build a graph with the following syntax:

    Graph(something, format="elliptic_curve_congruence")

but it would be more sensible to move the relevant block of 30 lines of code into the elliptic curves module.

Change History (18)

comment:1 Changed 5 years ago by cremona

  • Branch set to u/cremona/19382
  • Commit set to 47e4456b02bf7bab4fe9a828d93c5d8ba68e5132

Here is a first version anyway -- the old code was very broken, and I have not tested that this displays the correct graph, but at least it works. Right now I am working from a remote desktop so cannot easily display graphs; when I can, I will test it further.


New commits:

47e4456#19382 move elliptic curve congruence graphs

comment:2 Changed 5 years ago by ncohen

Hello John,

The ticket #19381 has been reviewed much faster than I expected, so tell me when you are done with this ticket and I will rebase it on top of it. Should be totally straightforward :-)

Nathann

comment:3 Changed 5 years ago by cremona

I am done with it. Can you test it by running the code in the doctest and checking that the graph does display? We can come back and improve it later.

comment:4 Changed 5 years ago by ncohen

Well, the doctests do pass and I can plot the graph, no problem. Except that I do not know what it means ^^;

Nathann

comment:5 Changed 5 years ago by cremona

  • Status changed from new to needs_review

comment:6 Changed 5 years ago by cremona

I'll ask William to take a look as I suspect he had something to do with the original (if not the code ;))

comment:7 Changed 5 years ago by ncohen

  • Dependencies set to #19381

Surprisingly there is no conflict with #19381 :-)

Nathann

comment:8 Changed 5 years ago by chapoton

  • Branch changed from u/cremona/19382 to public/19382
  • Commit changed from 47e4456b02bf7bab4fe9a828d93c5d8ba68e5132 to 01ead9fa0ab4d30212ff9376c3c4b7ea4f540579
  • Dependencies #19381 deleted

New commits:

01ead9fMerge branch 'u/cremona/19382' into 6.10.b2

comment:9 Changed 5 years ago by chapoton

  • Milestone changed from sage-6.9 to sage-6.10

comment:10 Changed 5 years ago by git

  • Commit changed from 01ead9fa0ab4d30212ff9376c3c4b7ea4f540579 to ce78d93c96ce657123a860ce8ba01ae958e8040b

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

ce78d93trac #19382 details (pep8, etc)

comment:11 Changed 5 years ago by git

  • Commit changed from ce78d93c96ce657123a860ce8ba01ae958e8040b to 96b473af366e3753a379284bdf3b33cc094fe133

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

5f3e925Merge branch 'public/19382' of git://trac.sagemath.org/sage into 19382
96b473atrac #19382 fixing the code

comment:12 Changed 5 years ago by chapoton

  • Status changed from needs_review to positive_review

ok, I propose to close this in the current state.

comment:13 Changed 5 years ago by chapoton

  • Reviewers set to Frédéric Chapoton

comment:14 Changed 5 years ago by git

  • Commit changed from 96b473af366e3753a379284bdf3b33cc094fe133 to 5607dac76b7409d68a72d501de00339b3bdbc6d2
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

5607dactrac #19382: Leftover occurrence of 'elliptic_curve'

comment:15 Changed 5 years ago by ncohen

If you don't mind ^^;

comment:16 Changed 5 years ago by chapoton

yes, I hope that you have been fast enough..

comment:17 Changed 5 years ago by ncohen

  • Status changed from needs_review to positive_review

Well, if Volker merges the branch without this last commit nobody will suffer because of it. It should work, however. I'm setting this to positive_review again, hoping there's nothing wrong with that.

Nathann

comment:18 Changed 5 years ago by vbraun

  • Branch changed from public/19382 to 5607dac76b7409d68a72d501de00339b3bdbc6d2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.