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:  sage6.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 sagedevel:
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
 Branch set to u/cremona/19382
 Commit set to 47e4456b02bf7bab4fe9a828d93c5d8ba68e5132
comment:2 Changed 5 years ago by
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
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
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
 Status changed from new to needs_review
comment:6 Changed 5 years ago by
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
 Dependencies set to #19381
Surprisingly there is no conflict with #19381 :)
Nathann
comment:8 Changed 5 years ago by
 Branch changed from u/cremona/19382 to public/19382
 Commit changed from 47e4456b02bf7bab4fe9a828d93c5d8ba68e5132 to 01ead9fa0ab4d30212ff9376c3c4b7ea4f540579
 Dependencies #19381 deleted
New commits:
01ead9f  Merge branch 'u/cremona/19382' into 6.10.b2

comment:9 Changed 5 years ago by
 Milestone changed from sage6.9 to sage6.10
comment:10 Changed 5 years ago by
 Commit changed from 01ead9fa0ab4d30212ff9376c3c4b7ea4f540579 to ce78d93c96ce657123a860ce8ba01ae958e8040b
Branch pushed to git repo; I updated commit sha1. New commits:
ce78d93  trac #19382 details (pep8, etc)

comment:11 Changed 5 years ago by
 Commit changed from ce78d93c96ce657123a860ce8ba01ae958e8040b to 96b473af366e3753a379284bdf3b33cc094fe133
comment:12 Changed 5 years ago by
 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
 Reviewers set to Frédéric Chapoton
comment:14 Changed 5 years ago by
 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:
5607dac  trac #19382: Leftover occurrence of 'elliptic_curve'

comment:15 Changed 5 years ago by
If you don't mind ^^;
comment:16 Changed 5 years ago by
yes, I hope that you have been fast enough..
comment:17 Changed 5 years ago by
 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
 Branch changed from public/19382 to 5607dac76b7409d68a72d501de00339b3bdbc6d2
 Resolution set to fixed
 Status changed from positive_review to closed
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:
#19382 move elliptic curve congruence graphs