Opened 6 years ago

Closed 6 years ago

#14251 closed enhancement (fixed)

Circulant digraphs

Reported by: ncohen Owned by: jason, ncohen, rlm
Priority: major Milestone: sage-5.9
Component: graph theory Keywords:
Cc: dcoudert Merged in: sage-5.9.beta0
Authors: Nathann Cohen Reviewers: David Coudert
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

A new constructor for digraphs !

Attachments (2)

trac_14251.patch (3.1 KB) - added by ncohen 6 years ago.
trac_14251-input.patch (2.1 KB) - added by ncohen 6 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 6 years ago by ncohen

  • Status changed from new to needs_review

Changed 6 years ago by ncohen

comment:2 follow-up: Changed 6 years ago by dcoudert

  • Reviewers set to David Coudert
  • Status changed from needs_review to needs_work

Hello,

  • I have read somewhere that imports should now be at module level, but I'm not sure...
  • You should test that n>=0
  • You should check that integers is an iterable (list, set, dict?) with valid numbers (can we have negative numbers? what if a number is larger than n? what to do with 0?)

David.

comment:3 in reply to: ↑ 2 Changed 6 years ago by ncohen

Yo !

  • I have read somewhere that imports should now be at module level, but I'm not sure...

Cython imports should be, but that's all. Anyway you cannot have all imports at module level, it wouldn't work. You need to use graph generators in digraph.py, and you need to use digraphs indigraph_generators too.

  • You should test that n>=0

Why not...

  • You should check that integers is an iterable (list, set, dict?) with valid numbers (can we have negative numbers? what if a number is larger than n? what to do with 0?)

If it is not iterable the "for j in integers" would fail with a clear message :

sage: digraphs.Circulant(4,5)      
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-2-50bde61731c6> in <module>()
----> 1 digraphs.Circulant(Integer(4),Integer(5))

/home/ncohen/.Sage/local/lib/python2.7/site-packages/sage/graphs/digraph_generators.pyc in Circulant(self, n, integers)
    407 
    408         for v in range(n):
--> 409             G.add_edges([(v,(v+j)%n) for j in integers])
    410 
    411         return G

TypeError: 'sage.rings.integer.Integer' object is not iterable
sage:

I will add some checks.. And I really hate when a 3-lines code takes 10 lines to check bad input... Users should learn to deal with them too.

Nathann

Changed 6 years ago by ncohen

comment:4 Changed 6 years ago by ncohen

Done !

Nathann

comment:5 Changed 6 years ago by dcoudert

  • Status changed from needs_work to positive_review

That's much better ;-)

I have plenty of warnings when building the documentation (with 5.8.beta4), but this has nothing to do with your patch (a sample bellow).

For me the patch is good to go.

Sample of warnings:

[graphs   ] Exception OverflowError: 'long int too large to convert to int' in <sage.structure.coerce_dict.TripleDictEraser object at 
0x8c83f44> ignored

[misc     ] Exception OverflowError: 'long int too large to convert to int' in <sage.structure.coerce_dict.TripleDictEraser object at 
0x8c83f44> ignored

[notebook ] Exception OverflowError: 'long int too large to convert to int' in <sage.structure.coerce_dict.TripleDictEraser object at 
0x8c83f44> ignored

[categorie] Exception OverflowError: 'long int too large to convert to int' in <sage.structure.coerce_dict.TripleDictEraser object at 
0x8c83f44> ignored

[geometry ] Exception OverflowError: 'long int too large to convert to int' in <sage.structure.coerce_dict.TripleDictEraser object at 
0x8c83f44> ignored

[interface] Exception OverflowError: 'long int too large to convert to int' in <sage.structure.coerce_dict.TripleDictEraser object at 
0x8c83f44> ignored

[libs     ] Exception OverflowError: 'long int too large to convert to int' in <sage.structure.coerce_dict.TripleDictEraser object at 
0x8c83f44> ignored

[matrices ] Exception OverflowError: 'long int too large to convert to int' in <sage.structure.coerce_dict.TripleDictEraser object at 
0x8c83f44> ignored

[rings_sta] Exception OverflowError: 'long int too large to convert to int' in <sage.structure.coerce_dict.TripleDictEraser object at 0xa7d0f44> ignored

[structure] Exception OverflowError: 'long int too large to convert to int' in <sage.structure.coerce_dict.TripleDictEraser object at 0xa7d0f44> ignored

comment:6 Changed 6 years ago by ncohen

Thaaaaaaanks !!! And I don't think that the doc warnings are related either. Actually the doc is such a mess these days... I'm eager to see it turn back to normal. It's almost impossible t check the doc of a patch for me right now :-/

Nathann

comment:7 Changed 6 years ago by jdemeyer

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