Ticket #13088 (closed enhancement: fixed)
implement the Paley graphs
| Reported by: | chapoton | Owned by: | jason, ncohen, rlm |
|---|---|---|---|
| Priority: | major | Milestone: | sage-5.2 |
| Component: | graph theory | Keywords: | graphs |
| Cc: | Work issues: | ||
| Report Upstream: | N/A | Reviewers: | Nathann Cohen |
| Authors: | Frédéric Chapoton | Merged in: | sage-5.2.rc0 |
| Dependencies: | #12989, #13038, #13058 | Stopgaps: |
Description
part of the bigger project #9136
This provides the familly of Paley graphs
Attachments
-
trac_13088-doctest.patch
(796 bytes) -
added by ncohen 13 months ago.
-
trac_13088_paley_graphs.patch
(1.8 KB) -
added by jdemeyer 12 months ago.
Change History
comment:2 Changed 13 months ago by ncohen
- Reviewers set to Nathann Cohen
- Dependencies set to 13058
- Authors set to Frédéric Chapoton
Goooooooood to go !! I added a doctest to your method.. If you agree with this change, can you set the ticket to positive_review ? :-)
Thaaaaaaaaaanks !!
Nathann
comment:3 Changed 13 months ago by chapoton
Hello Nathann,
Well, still two points that bother me :
- Do we really need to test that q is a prime power ? This is already done by FiniteField? itself.
- Do you need to recompute PaleyGraph?(9) in your new doctest ?
What do you think ?
comment:4 Changed 13 months ago by ncohen
Hellooooooooooooooo !!!
Oh, you are right, the graph already exists at this moment, so I should not create it anew ! Patch updated.
About q being a prime power or not : you do not "need" to test it, for the code would throw an exception indeed when calling FiniteField?, but the message would be less clear for somebody trying to create a PaleyGraph?. And nobody is hurt if it stays in the code :-)
Nathann
comment:5 Changed 13 months ago by chapoton
Hum, this will not work, because I call it capital G and you call it small g...
comment:6 Changed 13 months ago by ncohen
Ahahaah. Updated :-)
sage -t "devel/sage-2/sage/graphs/graph_generators.py" [26.1 s] ---------------------------------------------------------------------- All tests passed! Total time for all tests: 26.1 seconds
Nathann
comment:7 Changed 13 months ago by chapoton
- Status changed from needs_review to positive_review
But what about this strange dependency, without # in front ? I suggest to remove it.
comment:8 Changed 13 months ago by ncohen
Oh. I added the dependency because there are maaaaaaaaaaaaaaaaaaaaaaany patches that got reviewed recently that touch this file, and it would not be very funny to rebase them all :-D
Nathann
comment:10 Changed 13 months ago by chapoton
Well, too bad, now that the dependency is set, the patch does no longer apply smoothly. I suppose that it needs to be rebased, but I am not able to do that.
comment:11 Changed 13 months ago by ncohen
? Home come ? I applied it on top of all of them O_o
Nathann
comment:12 Changed 13 months ago by chapoton
Well, maybe I am wrong.. I am just not happy with the bot turning red..
comment:13 Changed 13 months ago by ncohen
Well, it applies fine on my version of Sage... Did you also apply all the dependencies of #13058 ?
Anyway some of them will probably have to be rebased before being merged, depending on the order in which Jeroen will pick them.
Nathann
comment:15 Changed 12 months ago by jdemeyer
- Dependencies changed from #13058 to #12989, #13038, #13058
comment:16 Changed 11 months ago by jdemeyer
- Status changed from positive_review to closed
- Resolution set to fixed
- Merged in set to sage-5.2.rc0
