Opened 6 years ago

Last modified 6 years ago

#18185 closed defect

Clean the Graph/DiGraph constructors — at Version 11

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:
Report Upstream: N/A Work issues:
Branch: public/18185 (Commits, GitHub, GitLab) Commit: f8c9f8ef8662324dee8f1bc403968fa5b2d168aa
Dependencies: #18067 Stopgaps:

Status badges

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 (11)

comment:1 Changed 6 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 6 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 6 years ago by ncohen

(rebased on latest beta)

comment:4 Changed 6 years ago by chapoton

  • Status changed from needs_review to needs_work

syntax error (missing : see patchbot report )

elif format == 'NX'

comment:5 Changed 6 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 6 years ago by ncohen

  • Status changed from needs_work to needs_review

comment:7 Changed 6 years ago by chapoton

  • Status changed from needs_review to needs_work

many doctests are failing, see patchbot report..

comment:8 Changed 6 years ago by ncohen

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

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

  • Status changed from needs_work to needs_review

comment:11 Changed 6 years ago by ncohen

  • Description modified (diff)
Note: See TracTickets for help on using tickets.