Opened 7 years ago
Closed 7 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)
Change History (9)
comment:1 Changed 7 years ago by
- Status changed from new to needs_review
Changed 7 years ago by
comment:2 follow-up: ↓ 3 Changed 7 years ago by
- Reviewers set to David Coudert
- Status changed from needs_review to needs_work
comment:3 in reply to: ↑ 2 Changed 7 years ago by
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 7 years ago by
comment:4 Changed 7 years ago by
Done !
Nathann
comment:5 Changed 7 years ago by
- 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 7 years ago by
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 7 years ago by
- Merged in set to sage-5.9.beta0
- Resolution set to fixed
- Status changed from positive_review to closed
Hello,
David.