Opened 5 years ago

Closed 5 years ago

#16398 closed defect (fixed)

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

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.3
Component: graph theory Keywords:
Cc: Merged in:
Authors: Nathann Cohen Reviewers: Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: fa1b01a (Commits) Commit: fa1b01a5578af2d4cae18d5542638293d5460864
Dependencies: Stopgaps:

Description (last modified by ncohen)

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

Change History (13)

comment:1 Changed 5 years ago by ncohen

  • Branch set to u/ncohen/16398
  • Commit set to 88474db5f0c1ef3f7a7d3d1b8f23b9ff14eb86d0
  • Status changed from new to needs_review

New commits:

88474dbtrac #16398: Cleaning the Graph documentation index

comment:2 follow-up: Changed 5 years ago by chapoton

  • Branch changed from u/ncohen/16398 to public/ticket/16398
  • Commit changed from 88474db5f0c1ef3f7a7d3d1b8f23b9ff14eb86d0 to 73a465b8a250455555ae122a616382ce49503c95
  • Status changed from needs_review to needs_info

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

comment:3 in reply to: ↑ 2 Changed 5 years ago by ncohen

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

comment:4 follow-up: Changed 5 years ago by chapoton

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.

comment:5 in reply to: ↑ 4 Changed 5 years ago by ncohen

  • Description modified (diff)
  • Status changed from needs_info to needs_review
  • Summary changed from Cleaning the Graph documentation index to Cleaning the Graph documentation index + remove numerical/test.py

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

comment:6 follow-up: Changed 5 years ago by chapoton

  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_review to positive_review

Thanks

comment:7 in reply to: ↑ 6 Changed 5 years ago by ncohen

Thanks

That's my line :-)

Nathann

comment:8 Changed 5 years ago by vbraun

  • Status changed from positive_review to needs_work
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]

comment:9 Changed 5 years ago by git

  • Commit changed from 73a465b8a250455555ae122a616382ce49503c95 to 155eb0e878ea186c130c2b7c7faaf155ea9226ee

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

155eb0etrac #16398 correct failing doctest

comment:10 Changed 5 years ago by chapoton

  • Status changed from needs_work to positive_review

Sorry, for that. The doctest is now corrected.

comment:11 Changed 5 years ago by git

  • Commit changed from 155eb0e878ea186c130c2b7c7faaf155ea9226ee to fa1b01a5578af2d4cae18d5542638293d5460864
  • Status changed from positive_review to needs_review

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

comment:12 Changed 5 years ago by ncohen

  • Status changed from needs_review to positive_review

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

comment:13 Changed 5 years ago by vbraun

  • Branch changed from public/ticket/16398 to fa1b01a5578af2d4cae18d5542638293d5460864
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.