Opened 7 years ago
Closed 7 years ago
#18185 closed defect (fixed)
Clean the Graph/DiGraph constructors
Reported by:  ncohen  Owned by:  

Priority:  major  Milestone:  sage6.7 
Component:  graph theory  Keywords:  
Cc:  ncohen, sagecombinat, tmonteil, vdelecroix, dcoudert, darij  Merged in:  
Authors:  Nathann Cohen  Reviewers:  David Coudert 
Report Upstream:  N/A  Work issues:  
Branch:  6afd117 (Commits, GitHub, GitLab)  Commit:  6afd117df805ce5b42c8019069824ed0cd0d277f 
Dependencies:  #18067  Stopgaps: 
Description (last modified by )
The Graph and DiGraph? constructor are rather long, and rather messy, as they are meant to accept a LOT of different inputs.
The point of this branch is to simplify its logic, in order to be able to split it into subfunctions later.
Here is the current logic: 1) Detect and set the 'format' if it was not specified 2) For every possible format, define whether the graph should allow loops/multiedges 3) Pick a data structure (i.e. backend) and create it 4) For every possible format, add vertices and edges
Besides local fixes and modifications, this branch moves '3' right after '1'. Then, '2' and '4' are merged so that we only enumerate all possible formats once.
I tried to split commits to make it easier to review. I know that it will not be very easy to read. It was not exactly easy to write either.
Change History (18)
comment:1 Changed 7 years ago by
 Branch set to public/18185
 Commit set to 279b141566b69d149e89992dba2fac74399815c9
 Status changed from new to needs_review
comment:2 Changed 7 years ago by
 Commit changed from 279b141566b69d149e89992dba2fac74399815c9 to 4b15f0f7f1cebd6f858e49ae2bf952dec6a2544f
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
6f4dbfa  trac #18185: Code simplifications (and fix)

b03266d  trac #18185: Rename graph6related functions (+doc)

b9cfde3  trac #18185: Move backend creation to the beginning of Graph.__init__ (does not pass tests)

050b916  trac #18185: Fix and simplify the code (now tests pass again)

d5c0812  trac #18185: Move backend creation to the beginning of DiGraph.__init__ (does not pass tests)

313810e  trac #18185: Fix and simplify the code (now tests pass again)

1f2e0fa  trac #18185: Merge the last two sections of the graph constructor

4b15f0f  trac #18185: Merge the last two sections of the DiGraph constructor

comment:3 Changed 7 years ago by
(rebased on latest beta)
comment:4 Changed 7 years ago by
 Status changed from needs_review to needs_work
syntax error (missing : see patchbot report )
elif format == 'NX'
comment:5 Changed 7 years ago by
 Commit changed from 4b15f0f7f1cebd6f858e49ae2bf952dec6a2544f to 80456d80dafe26adf7ac95e71311c767ca5e866b
comment:6 Changed 7 years ago by
 Status changed from needs_work to needs_review
comment:7 Changed 7 years ago by
 Status changed from needs_review to needs_work
many doctests are failing, see patchbot report..
comment:8 Changed 7 years ago by
Funny. No problem in graphs/ and 1000 combinat modules break. Let's see...
comment:9 Changed 7 years ago by
 Commit changed from 80456d80dafe26adf7ac95e71311c767ca5e866b to f8c9f8ef8662324dee8f1bc403968fa5b2d168aa
Branch pushed to git repo; I updated commit sha1. New commits:
f8c9f8e  trac #18185: combinat decided that we can't sort vertices in DiGraph.__init__

comment:10 Changed 7 years ago by
 Status changed from needs_work to needs_review
comment:11 Changed 7 years ago by
 Description modified (diff)
comment:12 Changed 7 years ago by
 Milestone changed from sage6.6 to sage6.7
remains 4 doctests failure, see patchbot report
comment:13 Changed 7 years ago by
 Commit changed from f8c9f8ef8662324dee8f1bc403968fa5b2d168aa to 6afd117df805ce5b42c8019069824ed0cd0d277f
Branch pushed to git repo; I updated commit sha1. New commits:
6afd117  trac #18185: broken import

comment:14 Changed 7 years ago by
Heyyyyyyy guys. It would be cool if you could help me with this ticket, because there is not much one could do with the Graph class while this is waiting for a review (and so subject to changes).
Thanks,
Nathann
comment:15 Changed 7 years ago by
 Reviewers set to David Coudert
 Status changed from needs_review to positive_review
Hello,
Although the graph and digraph constructor remains hard to read (very long), the modifications you did are very helpful.
The doc build properly, and I have no doctest error. Good to go.
David.
comment:16 Changed 7 years ago by
Wouhouuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuu !!
Thanks for that review !!!!!
Nathann
comment:17 Changed 7 years ago by
Thank you, both of you!
comment:18 Changed 7 years ago by
 Branch changed from public/18185 to 6afd117df805ce5b42c8019069824ed0cd0d277f
 Resolution set to fixed
 Status changed from positive_review to closed
Last 10 new commits:
trac #18067: Merged with rc2
Code simplifications (and fix)
Rename graph6related functions (+doc)
Merge branch 'd' into CURRENT
Move backend creation to the beginning of Graph.__init__ (does not pass tests)
Fix and simplify the code (now tests pass again)
Move backend creation to the beginning of DiGraph.__init__ (does not pass tests)
Fix and simplify the code (now tests pass again)
Merge the last two sections of the graph constructor
Merge the last two sections of the DiGraph constructor