Opened 9 years ago

Closed 9 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:

Status badges

Description

part of the bigger project #9136

This provides the familly of Paley graphs

Attachments (2)

trac_13088-doctest.patch (796 bytes) - added by ncohen 9 years ago.
trac_13088_paley_graphs.patch (1.8 KB) - added by jdemeyer 9 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 9 years ago by chapoton

  • Status changed from new to needs_review

comment:2 Changed 9 years ago by ncohen

  • Authors set to Frédéric Chapoton
  • Dependencies set to 13058
  • Reviewers set to Nathann Cohen

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 9 years 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 9 years 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 9 years ago by chapoton

Hum, this will not work, because I call it capital G and you call it small g...

comment:6 Changed 9 years 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

Changed 9 years ago by ncohen

comment:7 Changed 9 years 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 9 years 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:9 Changed 9 years ago by chapoton

  • Dependencies changed from 13058 to #13058

comment:10 Changed 9 years 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 9 years ago by ncohen

? Home come ? I applied it on top of all of them O_o

Nathann

comment:12 Changed 9 years ago by chapoton

Well, maybe I am wrong.. I am just not happy with the bot turning red..

comment:13 Changed 9 years 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:14 Changed 9 years ago by jdemeyer

  • Milestone changed from sage-5.1 to sage-5.2

comment:15 Changed 9 years ago by jdemeyer

  • Dependencies changed from #13058 to #12989, #13038, #13058

Changed 9 years ago by jdemeyer

comment:16 Changed 9 years ago by jdemeyer

  • Merged in set to sage-5.2.rc0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.