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

Cleaning the Graph documentation index + remove numerical/test.py #16398

Closed
nathanncohen mannequin opened this issue May 25, 2014 · 21 comments
Closed

Cleaning the Graph documentation index + remove numerical/test.py #16398

nathanncohen mannequin opened this issue May 25, 2014 · 21 comments

Comments

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented May 25, 2014

Some trivial changes are done in the documentation of graphs. The file numerical/test.py is also removed, as it seems to be a legacy of a previous era. It does not even test Sage but scipy, it appears nowhere and is not properly indented. And if we keep things whenever we do not know what it is there for we will end up with a lot of useless code :-P

Component: graph theory

Author: Nathann Cohen

Branch/Commit: fa1b01a

Reviewer: Frédéric Chapoton

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

@nathanncohen nathanncohen mannequin added this to the sage-6.3 milestone May 25, 2014
@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented May 25, 2014

New commits:

88474dbtrac #16398: Cleaning the Graph documentation index

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented May 25, 2014

Commit: 88474db

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented May 25, 2014

Branch: u/ncohen/16398

@nathanncohen nathanncohen mannequin added the s: needs review label May 25, 2014
@fchapoton
Copy link
Contributor

comment:2

Hello Nathann,

I have rebased on 6.3.beta3, and things look good to me, but I do not understand why you remove completely the file src/sage/numerical/test.py

Could you please explain why, or repair that if this was a mistake ?


New commits:

73a465bMerge branch 'u/ncohen/16398' and 6.3.beta6

@fchapoton
Copy link
Contributor

Changed commit from 88474db to 73a465b

@fchapoton
Copy link
Contributor

Changed branch from u/ncohen/16398 to public/ticket/16398

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Jul 27, 2014

comment:3

Helloooooooooooooo !!

I have rebased on 6.3.beta3, and things look good to me, but I do not understand why you remove completely the file src/sage/numerical/test.py

Could you please explain why, or repair that if this was a mistake ?

I don't think I did this by mistake (I had mostly forgotten this patch :-P) but really I don't know what to do with this file.

  1. It does not appear in the doc
  2. It contains nothing but a doctest (not properly indented. I just checked that it was actually doctested when you do sage -t test.py and apparently it does)
  3. It does not seem to test Sage but scipy
  4. All the modifications done to this file since the beginning are just people changing the output of the doctests when some updates are made, or Jeroen adding a "# long time" flag.

Here's what I think: this file has been added in 2007 and nobody knows what it does anymore. If we don't remove it it will just stay there forever, just because nobody knows what exactly it does. At the very least we should move its content to an existing file somewhere, but really I think we should remove it as nobody knows what exactly it does.

Nathann

@fchapoton
Copy link
Contributor

comment:4

Ok. I do not like when a ticket for something is used for something else. It reminds me of the national assembly using a law about agriculture to change the prize of the stamps, or something like that.

Nevertheless, I suppose that in the current case, I can still give a positive review, once the ticket description contains a complete and precise list of what it does, and the title is also changed accordingly.

@nathanncohen

This comment has been minimized.

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Jul 27, 2014

comment:5

Ok. I do not like when a ticket for something is used for something else. It reminds me of the national assembly using a law about agriculture to change the prize of the stamps, or something like that.

HMmmmm..... Only if you create a ticket for everything you end up with tickets doing really really stupid things. To me some things are not worth a ticket.

Nevertheless, I suppose that in the current case, I can still give a positive review, once the ticket description contains a complete and precise list of what it does, and the title is also changed accordingly.

Okayokay...

Nathann

@nathanncohen nathanncohen mannequin changed the title Cleaning the Graph documentation index Cleaning the Graph documentation index + remove numerical/test.py Jul 27, 2014
@fchapoton
Copy link
Contributor

comment:6

Thanks

@fchapoton
Copy link
Contributor

Reviewer: Frédéric Chapoton

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Jul 27, 2014

comment:7

Thanks

That's my line :-)

Nathann

@vbraun
Copy link
Member

vbraun commented Jul 28, 2014

comment:8
sage -t --long src/sage/graphs/digraph_generators.py
**********************************************************************
File "src/sage/graphs/digraph_generators.py", line 12, in sage.graphs.digraph_generators
Failed example:
    p = graphs.Circulant(10,[2,3])
Exception raised:
    Traceback (most recent call last):
      File "/scratch/buildbot/sage/redhawk-1/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 480, in _run
        self.execute(example, compiled, test.globs)
      File "/scratch/buildbot/sage/redhawk-1/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 839, in execute
        exec compiled in globs
      File "<doctest sage.graphs.digraph_generators[1]>", line 1, in <module>
        p = graphs.Circulant(Integer(10),[Integer(2),Integer(3)])
      File "lazy_import.pyx", line 326, in sage.misc.lazy_import.LazyImport.__getattr__ (build/cythonized/sage/misc/lazy_import.c:2837)
    AttributeError: GraphGenerators instance has no attribute 'Circulant'
**********************************************************************
1 item had failures:
   1 of   3 in sage.graphs.digraph_generators
    [81 tests, 1 failure, 8.25 s]

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 28, 2014

Changed commit from 73a465b to 155eb0e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 28, 2014

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

155eb0etrac #16398 correct failing doctest

@fchapoton
Copy link
Contributor

comment:10

Sorry, for that. The doctest is now corrected.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 28, 2014

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. This was a forced push. New commits:

fa1b01atrac #16398: broken doctest

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 28, 2014

Changed commit from 155eb0e to fa1b01a

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Jul 28, 2014

comment:12

(I updated it again: this is the file for digraphs constructor, so the fix was graph->digraph ^^; Thank you very much though !)

@vbraun
Copy link
Member

vbraun commented Jul 28, 2014

Changed branch from public/ticket/16398 to fa1b01a

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

2 participants