Opened 3 years ago

Closed 3 years ago

#27399 closed defect (fixed)

Expected ValueError when calling set_vertex()

Reported by: gh-Rithesh17 Owned by: gh-Rithesh17
Priority: minor Milestone: sage-8.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:

Status badges

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 gh-Rithesh17

  • Branch set to u/gh-Rithesh17/set_vertices-modified
  • Commit set to 32c10456a9b87537d5d77d97186976a5b5838a3b
  • Status changed from new to needs_review

New commits:

0ff156fPolyhedron documen modified
bba6a4aPolyhedron document modified.
32c1045set_vertices module in generic_graph.py modified

comment:2 Changed 3 years ago by chapoton

The branch is not clean, but it is mixed up with another branch.

comment:3 Changed 3 years ago by gh-Rithesh17

  • Branch changed from u/gh-Rithesh17/set_vertices-modified to u/gh-Rithesh17/set_vertex-modified
  • Commit changed from 32c10456a9b87537d5d77d97186976a5b5838a3b to 8ac6371ab8f56fa7db62576b7fdfd6552e094e14
  • Owner changed from (none) to gh-Rithesh17

New commits:

8ac6371set_vertex method in generic_graph.py modified

comment:4 Changed 3 years ago by dcoudert

Welcome to Sagemath!

The following changes are needed in your patch:

  • don't use not self._backend.has_vertex(vertex) but not self.has_vertex(vertex) or vertex 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 gh-Rithesh17

  • Branch changed from u/gh-Rithesh17/set_vertex-modified to u/gh-Rithesh17/set_vertex-mod
  • 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 2019-03-04-00-34-10-23cf6891.
Git branch: set_vertex-mod
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:

feec578set_vertex method modified
dae0b72set_vertex method modified

comment:6 Changed 3 years ago by dcoudert

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 git

  • Commit changed from dae0b72f6ab077299d92b242ee4a459e8fca8b7b to 34b1f6031df676628c6e2e00f814018b74b447a6

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

8ac6371set_vertex method in generic_graph.py modified
6582adbset_vertex method modified
34b1f60set_vertex method modified

comment:8 Changed 3 years ago by dcoudert

  • 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 git

  • Commit changed from 34b1f6031df676628c6e2e00f814018b74b447a6 to 94ab6e8c2dae56e62f726a1569e9fdea0f607716

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

9ce02e6set_vertex method modified
94ab6e8set_vertex method modified

comment:10 Changed 3 years ago by dcoudert

  • Status changed from needs_review to positive_review

For me this patch is good to go.

comment:11 Changed 3 years ago by vbraun

  • Branch changed from u/gh-Rithesh17/set_vertex-mod to 94ab6e8c2dae56e62f726a1569e9fdea0f607716
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.