Opened 10 years ago
Closed 10 years ago
#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: | Merged in: | sage-5.2.rc0 | |
Authors: | Frédéric Chapoton | Reviewers: | Nathann Cohen |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #12989, #13038, #13058 | Stopgaps: |
Description
part of the bigger project #9136
This provides the familly of Paley graphs
Attachments (2)
Change History (18)
comment:1 Changed 10 years ago by
- Status changed from new to needs_review
comment:2 Changed 10 years ago by
- Dependencies set to 13058
- Reviewers set to Nathann Cohen
comment:3 Changed 10 years ago by
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 10 years ago by
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 10 years ago by
Hum, this will not work, because I call it capital G and you call it small g...
comment:6 Changed 10 years ago by
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
Changed 10 years ago by
comment:7 Changed 10 years ago by
- 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 10 years ago by
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:9 Changed 10 years ago by
- Dependencies changed from 13058 to #13058
comment:10 Changed 10 years ago by
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 10 years ago by
? Home come ? I applied it on top of all of them O_o
Nathann
comment:12 Changed 10 years ago by
Well, maybe I am wrong.. I am just not happy with the bot turning red..
comment:13 Changed 10 years ago by
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:14 Changed 10 years ago by
- Milestone changed from sage-5.1 to sage-5.2
comment:15 Changed 10 years ago by
- Dependencies changed from #13058 to #12989, #13038, #13058
Changed 10 years ago by
comment:16 Changed 10 years ago by
- Merged in set to sage-5.2.rc0
- Resolution set to fixed
- Status changed from positive_review to closed
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