Opened 3 years ago
Closed 3 years ago
#27399 closed defect (fixed)
Expected ValueError when calling set_vertex()
Reported by:  ghRithesh17  Owned by:  ghRithesh17 

Priority:  minor  Milestone:  sage8.7 
Component:  graph theory  Keywords:  
Cc:  Merged in:  
Authors:  Rithesh K  Reviewers:  David Coudert 
Report Upstream:  N/A  Work issues:  
Branch:  94ab6e8 (Commits, GitHub, GitLab)  Commit:  94ab6e8c2dae56e62f726a1569e9fdea0f607716 
Dependencies:  Stopgaps: 
Description
This is the issue discussed in the comments section of the ticket https://trac.sagemath.org/ticket/14708:
sage: g = Graph() sage: g.set_vertex('foo', 'bar') sage: g.get_vertices() {}
We should have expected a ValueError? in the set_vertex()
method. Also, on continuing,
sage: g.add_vertex('foo') sage: g.get_vertices() {'foo': 'bar'}
Which shouldn't have happened
Change History (11)
comment:1 Changed 3 years ago by
 Branch set to u/ghRithesh17/set_verticesmodified
 Commit set to 32c10456a9b87537d5d77d97186976a5b5838a3b
 Status changed from new to needs_review
comment:2 Changed 3 years ago by
The branch is not clean, but it is mixed up with another branch.
comment:3 Changed 3 years ago by
 Branch changed from u/ghRithesh17/set_verticesmodified to u/ghRithesh17/set_vertexmodified
 Commit changed from 32c10456a9b87537d5d77d97186976a5b5838a3b to 8ac6371ab8f56fa7db62576b7fdfd6552e094e14
 Owner changed from (none) to ghRithesh17
New commits:
8ac6371  set_vertex method in generic_graph.py modified

comment:4 Changed 3 years ago by
Welcome to Sagemath!
The following changes are needed in your patch:
 don't use
not self._backend.has_vertex(vertex)
butnot self.has_vertex(vertex)
orvertex not in self
 the error message is too long. You can reduce it as in method
delete_vertex
.  the error message is for vertex 4 in the doctest, not 1
so you must do:
 ValueError: vertex (1) not in the graph.  Please use add_vertex() method to add the vertex to the graph before setting it to the object. + ValueError: vertex (4) not in the graph
and
 if not self._backend.has_vertex(vertex):  raise ValueError('vertex (%s) not in the graph.\nPlease use add_vertex() method to add the vertex to the graph before setting it to the object.'%str(vertex)) + if vertex not in self: + raise ValueError('vertex (%s) not in the graph'%str(vertex))

To check that the changes you do are OK, you must run the doctests using for instance
sage btp long src/sage/graphs/generic_graph.py
comment:5 Changed 3 years ago by
 Branch changed from u/ghRithesh17/set_vertexmodified to u/ghRithesh17/set_vertexmod
 Commit changed from 8ac6371ab8f56fa7db62576b7fdfd6552e094e14 to dae0b72f6ab077299d92b242ee4a459e8fca8b7b
Thank you for the review. I have adopted all the suggestions provided.
I ran the doctests and this was the output:
Running doctests with ID 2019030400341023cf6891. Git branch: set_vertexmod Using optional=dochtml,gfortran,memlimit,mpir,python2,sage Doctesting 1 file using 4 threads. sage t long src/sage/graphs/generic_graph.py [3288 tests, 37.88 s]  All tests passed!  Total time for all tests: 38.8 seconds cpu time: 32.1 seconds cumulative wall time: 37.9 seconds
New commits:
feec578  set_vertex method modified

dae0b72  set_vertex method modified

comment:6 Changed 3 years ago by
A minor issue: remove the .
at the end of 'vertex (%s) not in the graph.'
and of course in the doctest.
This is PEP8.
comment:7 Changed 3 years ago by
 Commit changed from dae0b72f6ab077299d92b242ee4a459e8fca8b7b to 34b1f6031df676628c6e2e00f814018b74b447a6
comment:8 Changed 3 years ago by
 Reviewers set to David Coudert
Further small remarks:
 sage: T.set_vertex(4, graphs.DodecahedralGraph()) + sage: T.set_vertex(4, 'foo')
There is no need to build the graph here as we just want to show that it is not possible to set label of a non existing vertex. Running all doctests is long, so when we can illustrate an issue with a smaller/faster example, it's always better.
 raise ValueError('vertex (%s) not in the graph'%str(vertex)) + raise ValueError('vertex (%s) not in the graph' % str(vertex))
this is also PEP8.
comment:9 Changed 3 years ago by
 Commit changed from 34b1f6031df676628c6e2e00f814018b74b447a6 to 94ab6e8c2dae56e62f726a1569e9fdea0f607716
comment:10 Changed 3 years ago by
 Status changed from needs_review to positive_review
For me this patch is good to go.
comment:11 Changed 3 years ago by
 Branch changed from u/ghRithesh17/set_vertexmod to 94ab6e8c2dae56e62f726a1569e9fdea0f607716
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Polyhedron documen modified
Polyhedron document modified.
set_vertices module in generic_graph.py modified