Opened 7 years ago

Closed 7 years ago

#13862 closed enhancement (fixed)

Split graph_generators into several files

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

Description (last modified by jdemeyer)

This tickets addresses the fact that Jeroen is not happy with the length of graph_generators.py :-P

It splits the file into new ones, added in the graph/generators/ folder, according to the classifiation of the methods that appears in the module's documentation.

APPLY:

Attachments (7)

trac_13862.patch (572.2 KB) - added by ncohen 7 years ago.
trac_13862-cleaning_and_moving.patch (30.8 KB) - added by ncohen 7 years ago.
trac_13862-doc.patch (17.0 KB) - added by ncohen 7 years ago.
trac_13862-code_reformatting.patch (64.2 KB) - added by ncohen 7 years ago.
trac_13862-unapply_13809.patch (2.3 KB) - added by ncohen 7 years ago.
trac_13862-reapply_13809.patch (3.0 KB) - added by ncohen 7 years ago.
trac_13862-moves_3_constructors.patch (42.5 KB) - added by ncohen 7 years ago.

Download all attachments as: .zip

Change History (23)

comment:1 Changed 7 years ago by ncohen

  • Authors set to Nathann Cohen
  • Cc dcoudert added
  • Component changed from PLEASE CHANGE to graph theory
  • Owner changed from tbd to jason, ncohen, rlm
  • Type changed from PLEASE CHANGE to enhancement

comment:2 Changed 7 years ago by ncohen

  • Description modified (diff)

This patch creates the following files and moves into them the methods from GraphGenerator that are associated to it in the list at the top of the graph_generators module documentation.

  • basic.py
  • degree_sequence.py
  • families.py
  • platonic_solids.py
  • random.py
  • smallgraphs.py
  • world_map.py

This patch passes all tests, but is not clean yet and lacks documentations. Others will follow.

Nathann

Last edited 7 years ago by ncohen (previous) (diff)

Changed 7 years ago by ncohen

comment:3 Changed 7 years ago by ncohen

What this additional (trac_13862-cleaning_and_moving.patch) patch does :

  • Moves _circle_embedding and _line_embedding to graph_plot.py. These methods are now imported at the beginning of generators/* modules that need need.
  • Removes LCFGraph from the list of basc graphs (this methods creates a family of graphs). It was already in the list of "families" graphs.
  • Moves the Harary Graph to the "families" file. Same thing here : there are many Harary graphs.
  • Moves DorogovtsevGoltsevMendesGraph to families.py. Not worth creating an independent "pseudofractal" module at the moment.
  • Moves IntervalGraph to families. This one was not liste among the "families", which is why it was still in graph_generators.py after the first patch.
  • Adds the Nauru Graph to the list of small graphs.

Some work on the doc is still needed.

Nathann

Changed 7 years ago by ncohen

comment:4 Changed 7 years ago by ncohen

Apply trac_13862.patch, trac_13862-cleaning_and_moving.patch

comment:5 Changed 7 years ago by ncohen

  • Description modified (diff)

comment:6 Changed 7 years ago by ncohen

  • Description modified (diff)

What this new patch (trac_13862-doc.patch) does :

  • Moves the "Usage" part of the graph_generators module's doc to graph_plot, as it explains how graphs are displayed.
  • *HEAVY* rewrite of the fuctions lists at the top of graph_generators. Now the methods are listed in 3xN tables, so that they are easier to browse. This required to define an (ugly) helper function in graph_generators.py. But the lists at the top of that file are now easier to read (both in html and in sage.graphs.graph_generators?) and easier to update (just add a string in a list).
  • NauruGraph isadded to the list of small graphs

This patch's almost ready.

Nathann

Changed 7 years ago by ncohen

comment:7 Changed 7 years ago by ncohen

  • Description modified (diff)

What this patch does (trac_13862-code_reformatting.patch):

  • Removes all occurrences to self, as the former methods are now functions and belong to no class.
  • Adds a line of doc to all new files, saying that their documentation actually appears in graph_generators

Lots of code formatting :-P

(only one patch left)

Nathann

Changed 7 years ago by ncohen

comment:8 Changed 7 years ago by ncohen

  • Description modified (diff)
  • Status changed from new to needs_review

What this patch does (trac_13862-moves_3_constructors.patch):

  • Moves CompleteGraph, CompleteBipartiteGraph and CompleteMultipartiteGraph from families.py to basic.py
  • Adds CompleteMultipartiteGraph to the lists, as it did not appears before O_o

And nooooow, this ticket is ready to be reviewed !!! :-P

Nathann

comment:9 Changed 7 years ago by dcoudert

  • Status changed from needs_review to needs_work

Hello,

the long tests fail on families.py

**********************************************************************
File "/path-to-sage/sage-5.6.beta0/devel/sage-myclone/sage/graphs/generators/families.py", line 1815:
    sage: G = graphs.RingedTree(5)
Exception raised:
    Traceback (most recent call last):
      File "/path-to-sage/sage-5.6.beta0/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/path-to-sage/sage-5.6.beta0/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/path-to-sage/sage-5.6.beta0/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_25[2]>", line 1, in <module>
        G = graphs.RingedTree(Integer(5))###line 1815:
    sage: G = graphs.RingedTree(5)
      File "/path-to-sage/sage-5.6.beta0/local/lib/python/site-packages/sage/graphs/generators/families.py", line 1845, in RingedTree
        from sage.graphs.graph_plot import GraphGenerators
    ImportError: cannot import name GraphGenerators

I have run tests only on the graph directory. I don't know if this patch could impact other modules.

Also, you should add possible dependencies with other patches e.g., #13809.

Changed 7 years ago by ncohen

Changed 7 years ago by ncohen

Changed 7 years ago by ncohen

comment:10 Changed 7 years ago by ncohen

  • Description modified (diff)
  • Status changed from needs_work to needs_review

God... I ran tests on these files a crazy amount of times but I forgot long tests :-P

Updated ! I cheated a bit by first removing #13809 then adding it again, but this set of patch is.... HEAVY to work with :-P

Nathann

Apply trac_13862-unapply_13809.patch, trac_13862.patch, trac_13862-cleaning_and_moving.patch, trac_13862-doc.patch, trac_13862-code_reformatting.patch, trac_13862-moves_3_constructors.patch, trac_13862-reapply_13809.patch

comment:11 Changed 7 years ago by dcoudert

  • Dependencies set to #13809
  • Status changed from needs_review to positive_review

Install OK, doctest OK including long tests, and docbuild OK. For me the patch is good to go!

I have added the dependency to the patch description.

This is definitely a huge patch, but it has to be done. Good job.

comment:12 Changed 7 years ago by ncohen

Wow. Glad to see that :-D

Thaaaaaaaaaaaaankss !!!

Nathann

comment:13 Changed 7 years ago by jdemeyer

  • Dependencies #13809 deleted
  • Description modified (diff)

comment:14 Changed 7 years ago by jdemeyer

  • Reviewers set to David Coudert

comment:15 Changed 7 years ago by ncohen

Apply trac_13862.patch, trac_13862-cleaning_and_moving.patch, trac_13862-doc.patch, trac_13862-code_reformatting.patch, trac_13862-moves_3_constructors.patch

comment:16 Changed 7 years ago by jdemeyer

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