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

Adds sage.graphs.base.graph_backend to the documentation #14805

Closed
nathanncohen mannequin opened this issue Jun 22, 2013 · 19 comments
Closed

Adds sage.graphs.base.graph_backend to the documentation #14805

nathanncohen mannequin opened this issue Jun 22, 2013 · 19 comments

Comments

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Jun 22, 2013

As the title says ! This patch does nothing smart.

Nathann


Apply to devel/sage: attachment: trac_14805.patch

Component: graph theory

Author: Nathann Cohen

Reviewer: Punarbasu Purkayastha

Merged: sage-5.12.beta0

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

@nathanncohen nathanncohen mannequin added this to the sage-5.12 milestone Jun 22, 2013
@ppurka
Copy link
Member

ppurka commented Jul 1, 2013

comment:3

Patchbot says "mmmm... tasty but can't digest; need rebase."

Edit: That's evil. You upload a new patch in the few seconds while I write. X-(

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Jul 1, 2013

comment:4

Sorrysorry I just did ! ;-)

Nathann

@ppurka
Copy link
Member

ppurka commented Jul 1, 2013

comment:5

Can you add the following:

  1. After every description iterator, add what it is an iterator of. In some cases it is clearly mentioned, whereas in other cases it is simply left as iterator.
  2. There are some INPUT descriptions like this: - ``new`` -- boolean or None or - ``new`` -- string or None . I think it should be explicitly mentioned that None corresponds to retrieving the current value. In fact, I am very surprised how it works. The function definition should be like the definition below so that G.loops() gives the value straightaway, without having to enter None as a function argument:
def loops(self, new=None):

It is a painful and extensive cleanup of that file. Thanks for the effort. :)

@ppurka
Copy link
Member

ppurka commented Jul 2, 2013

comment:6
  1. There is this sentence which needs spaces around the =, otherwise the rendered output is weird.
81	        If ``name``=``None``, the new vertex name is returned, ``None`` otherwise.

784	        If ``name``=``None``, the new vertex name is returned. ``None`` otherwise.
  1. This needs a double backticks; currently it is rendered in latex
201	            label of `(u,v)`
  1. It applied with one hunk at fuzz 2 against 5.11.beta3.

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Jul 2, 2013

comment:7

Hellooooooo !

I just updated the patch (sorry for the delay, I had some problems with Sage because of #14737), and fixed all your points.

Except point 4, where I removed the backticks instead of adding them : the whole file is full of (u,v) without backticks, and to me marking them with double backticks makes less sense than writing them as LaTeX characters. To me an edge is a math thing.

Well, what do you think ? We can make it Python, Maths, or let it stay like that too :-)

Nathann

@ppurka
Copy link
Member

ppurka commented Jul 2, 2013

comment:8

Only one correction to suggest. In the correction to point 2, the `None` should not be under single backticks. I am ok with removing the backticks from (u,v).

@ppurka
Copy link
Member

ppurka commented Jul 2, 2013

comment:9

Oh sorry. there is another one of the point 2 type.

1362	        - ``new`` -- string or None 

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Jul 2, 2013

comment:10

Gloops. Right. Patch updated !

Nathann

@ppurka
Copy link
Member

ppurka commented Jul 2, 2013

Reviewer: Punarbasu Purkayastha

@ppurka
Copy link
Member

ppurka commented Jul 2, 2013

comment:11

Attachment: trac_14805.patch.gz

Thanks. Looks good to me. :)

@ppurka

This comment has been minimized.

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Jul 2, 2013

comment:12

Thanks for the review :-)

Nathann

@ppurka
Copy link
Member

ppurka commented Jul 2, 2013

comment:13

Why is the patchbot unable to apply to 5.11b3? I applied to 5.11b3 and it worked except for the fuzz.

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Jul 2, 2013

comment:14

It is probably an old version. The one I uploaded when you quoted the patchbot, yesterday.

Nathann

@ppurka
Copy link
Member

ppurka commented Jul 2, 2013

comment:15

Let's kick the patchbot.

Apply trac_14805.patch

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Jul 2, 2013

comment:16

Underfed, then kicked.

We really should treat it better.

Nathann

@ppurka
Copy link
Member

ppurka commented Jul 2, 2013

comment:17

Replying to @nathanncohen:

Underfed, then kicked.

We really should treat it better.

Nathann

"C'est la vie" :-P

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Jul 2, 2013

comment:18

"C'est la vie" :-P

In life you have to kick back if people treat you like that :-P

Nathann

@jdemeyer
Copy link

jdemeyer commented Aug 2, 2013

Merged: sage-5.12.beta0

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