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:  sage6.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 )
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
 Branch set to u/ncohen/16398
 Commit set to 88474db5f0c1ef3f7a7d3d1b8f23b9ff14eb86d0
 Status changed from new to needs_review
comment:2 followup: ↓ 3 Changed 5 years ago by
 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:
73a465b  Merge branch 'u/ncohen/16398' and 6.3.beta6

comment:3 in reply to: ↑ 2 Changed 5 years ago by
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 followup: ↓ 5 Changed 5 years ago by
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
 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 followup: ↓ 7 Changed 5 years ago by
 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
Thanks
That's my line :)
Nathann
comment:8 Changed 5 years ago by
 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/redhawk1/sage_git/build/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 480, in _run self.execute(example, compiled, test.globs) File "/scratch/buildbot/sage/redhawk1/sage_git/build/local/lib/python2.7/sitepackages/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
 Commit changed from 73a465b8a250455555ae122a616382ce49503c95 to 155eb0e878ea186c130c2b7c7faaf155ea9226ee
Branch pushed to git repo; I updated commit sha1. New commits:
155eb0e  trac #16398 correct failing doctest

comment:10 Changed 5 years ago by
 Status changed from needs_work to positive_review
Sorry, for that. The doctest is now corrected.
comment:11 Changed 5 years ago by
 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:
fa1b01a  trac #16398: broken doctest

comment:12 Changed 5 years ago by
 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
 Branch changed from public/ticket/16398 to fa1b01a5578af2d4cae18d5542638293d5460864
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
trac #16398: Cleaning the Graph documentation index