Opened 5 years ago

Closed 4 years ago

#18185 closed defect (fixed)

Clean the Graph/DiGraph constructors

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.7
Component: graph theory Keywords:
Cc: ncohen, sage-combinat, tmonteil, vdelecroix, dcoudert, darij Merged in:
Authors: Nathann Cohen Reviewers: David Coudert
Report Upstream: N/A Work issues:
Branch: 6afd117 (Commits) Commit: 6afd117df805ce5b42c8019069824ed0cd0d277f
Dependencies: #18067 Stopgaps:

Description (last modified by ncohen)

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 5 years ago by ncohen

  • Branch set to public/18185
  • Commit set to 279b141566b69d149e89992dba2fac74399815c9
  • Status changed from new to needs_review

Last 10 new commits:

66478e5trac #18067: Merged with rc2
ebe615dCode simplifications (and fix)
85ef058Rename graph6-related functions (+doc)
5fe39bcMerge branch 'd' into CURRENT
e251a7bMove backend creation to the beginning of Graph.__init__ (does not pass tests)
0e63fcbFix and simplify the code (now tests pass again)
03fb9b2Move backend creation to the beginning of DiGraph.__init__ (does not pass tests)
4fbadc4Fix and simplify the code (now tests pass again)
8f34727Merge the last two sections of the graph constructor
279b141Merge the last two sections of the DiGraph constructor

comment:2 Changed 5 years ago by git

  • Commit changed from 279b141566b69d149e89992dba2fac74399815c9 to 4b15f0f7f1cebd6f858e49ae2bf952dec6a2544f

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

6f4dbfatrac #18185: Code simplifications (and fix)
b03266dtrac #18185: Rename graph6-related functions (+doc)
b9cfde3trac #18185: Move backend creation to the beginning of Graph.__init__ (does not pass tests)
050b916trac #18185: Fix and simplify the code (now tests pass again)
d5c0812trac #18185: Move backend creation to the beginning of DiGraph.__init__ (does not pass tests)
313810etrac #18185: Fix and simplify the code (now tests pass again)
1f2e0fatrac #18185: Merge the last two sections of the graph constructor
4b15f0ftrac #18185: Merge the last two sections of the DiGraph constructor

comment:3 Changed 5 years ago by ncohen

(rebased on latest beta)

comment:4 Changed 5 years ago by chapoton

  • Status changed from needs_review to needs_work

syntax error (missing : see patchbot report )

elif format == 'NX'

comment:5 Changed 5 years ago by git

  • Commit changed from 4b15f0f7f1cebd6f858e49ae2bf952dec6a2544f to 80456d80dafe26adf7ac95e71311c767ca5e866b

Branch pushed to git repo; I updated commit sha1. New commits:

313fd68trac #18185: Merged with beta1
80456d8trac 18185: missing ':'

comment:6 Changed 5 years ago by ncohen

  • Status changed from needs_work to needs_review

comment:7 Changed 5 years ago by chapoton

  • Status changed from needs_review to needs_work

many doctests are failing, see patchbot report..

comment:8 Changed 5 years ago by ncohen

Funny. No problem in graphs/ and 1000 combinat modules break. Let's see...

comment:9 Changed 5 years ago by git

  • Commit changed from 80456d80dafe26adf7ac95e71311c767ca5e866b to f8c9f8ef8662324dee8f1bc403968fa5b2d168aa

Branch pushed to git repo; I updated commit sha1. New commits:

f8c9f8etrac #18185: combinat decided that we can't sort vertices in DiGraph.__init__

comment:10 Changed 5 years ago by ncohen

  • Status changed from needs_work to needs_review

comment:11 Changed 5 years ago by ncohen

  • Description modified (diff)

comment:12 Changed 5 years ago by chapoton

  • Milestone changed from sage-6.6 to sage-6.7

remains 4 doctests failure, see patchbot report

comment:13 Changed 5 years ago by git

  • Commit changed from f8c9f8ef8662324dee8f1bc403968fa5b2d168aa to 6afd117df805ce5b42c8019069824ed0cd0d277f

Branch pushed to git repo; I updated commit sha1. New commits:

6afd117trac #18185: broken import

comment:14 Changed 4 years ago by ncohen

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 4 years ago by dcoudert

  • 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 4 years ago by ncohen

Wouhouuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuu !!

Thanks for that review !!!!!

Nathann

comment:17 Changed 4 years ago by darij

Thank you, both of you!

comment:18 Changed 4 years ago by vbraun

  • Branch changed from public/18185 to 6afd117df805ce5b42c8019069824ed0cd0d277f
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.