Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split graph_generators into several files #13862

Closed
nathanncohen mannequin opened this issue Dec 24, 2012 · 27 comments
Closed

Split graph_generators into several files #13862

nathanncohen mannequin opened this issue Dec 24, 2012 · 27 comments

Comments

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Dec 24, 2012

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:

CC: @dcoudert

Component: graph theory

Author: Nathann Cohen

Reviewer: David Coudert

Merged: sage-5.6.beta2

Issue created by migration from https://trac.sagemath.org/ticket/13862

@nathanncohen nathanncohen mannequin added this to the sage-5.6 milestone Dec 24, 2012
@nathanncohen nathanncohen mannequin added the p: major / 3 label Dec 24, 2012
@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Dec 24, 2012

Author: Nathann Cohen

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Dec 25, 2012

comment:2

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

@nathanncohen

This comment has been minimized.

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Dec 26, 2012

comment:3

Attachment: trac_13862.patch.gz

What this additional (attachment: 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

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Dec 26, 2012

Attachment: trac_13862-cleaning_and_moving.patch.gz

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Dec 26, 2012

comment:4

Apply trac_13862.patch, trac_13862-cleaning_and_moving.patch

@nathanncohen

This comment has been minimized.

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Dec 26, 2012

comment:6

What this new patch (attachment: 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

@nathanncohen

This comment has been minimized.

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Dec 26, 2012

Attachment: trac_13862-doc.patch.gz

@nathanncohen

This comment has been minimized.

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Dec 26, 2012

comment:7

What this patch does (attachment: 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

@nathanncohen

This comment has been minimized.

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Dec 26, 2012

comment:8

Attachment: trac_13862-code_reformatting.patch.gz

What this patch does (attachment: 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

@nathanncohen nathanncohen mannequin added the s: needs review label Dec 26, 2012
@dcoudert
Copy link
Contributor

comment:9

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.

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Dec 28, 2012

Attachment: trac_13862-unapply_13809.patch.gz

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Dec 28, 2012

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Dec 28, 2012

comment:10

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

@nathanncohen

This comment has been minimized.

@nathanncohen nathanncohen mannequin removed the s: needs work label Dec 28, 2012
@nathanncohen nathanncohen mannequin added the s: needs review label Dec 28, 2012
@dcoudert
Copy link
Contributor

comment:11

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.

@dcoudert
Copy link
Contributor

Dependencies: #13809

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Dec 29, 2012

comment:12

Wow. Glad to see that :-D

Thaaaaaaaaaaaaankss !!!

Nathann

@jdemeyer
Copy link

Changed dependencies from #13809 to none

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link

Reviewer: David Coudert

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Dec 30, 2012

comment:15

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

@jdemeyer
Copy link

jdemeyer commented Jan 1, 2013

Merged: sage-5.6.beta2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants