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

Adding new small graph structures #30159

Closed
Fightlapa mannequin opened this issue Jul 16, 2020 · 15 comments
Closed

Adding new small graph structures #30159

Fightlapa mannequin opened this issue Jul 16, 2020 · 15 comments

Comments

@Fightlapa
Copy link
Mannequin

Fightlapa mannequin commented Jul 16, 2020

CC: @dcoudert

Component: graph theory

Author: Jakub Jabłoński

Branch/Commit: 5ec6133

Reviewer: David Coudert

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

@Fightlapa Fightlapa mannequin added this to the sage-9.2 milestone Jul 16, 2020
@Fightlapa Fightlapa mannequin added p: major / 3 and removed p: major / 3 labels Jul 16, 2020
@Fightlapa
Copy link
Mannequin Author

Fightlapa mannequin commented Jul 16, 2020

Branch: public/graphs/new_basic_structures

@Fightlapa
Copy link
Mannequin Author

Fightlapa mannequin commented Jul 16, 2020

Commit: 23a7baa

@Fightlapa
Copy link
Mannequin Author

Fightlapa mannequin commented Jul 16, 2020

Author: Jakub Jabłoński

@Fightlapa
Copy link
Mannequin Author

Fightlapa mannequin commented Jul 16, 2020

New commits:

23a7baaAdding fork graph, gem graph and bull graph to graph list

@dcoudert
Copy link
Contributor

comment:7

Hello,

please do the following changes to the 3 methods.

  • use Return instead of Returns
-    Returns a gem graph with 5 nodes.
+    Returns a gem graph with 5 nodes.
  • this form is more usual. I know other forms are used in basic.py, but this will be unified in the future.
-    EXAMPLES: Construct and show a gem graph
-
-    ::
+    EXAMPLES:
+
+    Construct and show a gem graph::

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 18, 2020

Changed commit from 23a7baa to 1838d26

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 18, 2020

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

1838d26Fixed docs of new graph structures

@dcoudert
Copy link
Contributor

Reviewer: David Coudert

@dcoudert
Copy link
Contributor

comment:9

LGTM.

@vbraun
Copy link
Member

vbraun commented Aug 5, 2020

comment:10

See patchbot

@dcoudert
Copy link
Contributor

dcoudert commented Aug 5, 2020

comment:11

I didn't see that one, sorry

sage -t --long --random-seed=0 src/sage/graphs/graph_list.py
**********************************************************************
File "src/sage/graphs/graph_list.py", line 321, in sage.graphs.graph_list.show_graphs
Failed example:
    len(glist)
Expected:
    14
Got:
    17

Also, I suggest to add # long time after g.show() like

-        sage: g.show()
+        sage: g.show()  # long time

it's not interesting when running doctests to display these graphs, but it is good for the documentation.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 8, 2020

Changed commit from 1838d26 to 5ec6133

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 8, 2020

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

5ec6133Added long time comment in graph show method, fixed test

@dcoudert
Copy link
Contributor

dcoudert commented Aug 8, 2020

comment:13

LGTM.

@vbraun
Copy link
Member

vbraun commented Aug 10, 2020

Changed branch from public/graphs/new_basic_structures to 5ec6133

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