Opened 6 years ago
Closed 6 years ago
#20479 closed defect (fixed)
Correct error in vertex_boundary
Reported by:  dcoudert  Owned by:  

Priority:  major  Milestone:  sage7.2 
Component:  graph theory  Keywords:  
Cc:  jmantysalo, vdelecroix  Merged in:  
Authors:  David Coudert  Reviewers:  Jori Mäntysalo 
Report Upstream:  N/A  Work issues:  
Branch:  d35ce49 (Commits, GitHub, GitLab)  Commit:  d35ce496637639f29294fa6c369f78caad401b46 
Dependencies:  Stopgaps: 
Description
The documentation of vertex_boundary
says that If vertices2 is None, then vertices2 is the complement of vertices1. However this is not the case, as shown in this example.
sage: G = graphs.PathGraph(3) sage: G.vertex_boundary([0,1], vertices2=None) [0, 1, 2] sage: G.vertex_boundary([0,1], vertices2=set(G.vertices()).difference([0,1])) [2] sage: D = DiGraph(G) sage: D.vertex_boundary([0,1]) [0, 1, 2] sage: D.vertex_boundary([0,1], vertices2=set(D.vertices()).difference([0,1])) [2]
This patch solves this issue.
Change History (9)
comment:1 Changed 6 years ago by
 Branch set to u/dcoudert/20479
 Commit set to fb0deec2f4556c1d2c63f872f4390db5ba678e8a
 Status changed from new to needs_review
comment:2 Changed 6 years ago by
 Cc jmantysalo vdelecroix added
Simple patch that should be easy to review. Thanks.
comment:3 followup: ↓ 5 Changed 6 years ago by
References to corrected bugss should be in TESTS
section.
But more importantly, how this should work? If I understood correctly (have not compiled yet), then this will change current behaviour when vertices2=None
. This can surprise users.
comment:4 Changed 6 years ago by
 Commit changed from fb0deec2f4556c1d2c63f872f4390db5ba678e8a to d35ce496637639f29294fa6c369f78caad401b46
Branch pushed to git repo; I updated commit sha1. New commits:
d35ce49  trac #20479: add tests section

comment:5 in reply to: ↑ 3 ; followup: ↓ 6 Changed 6 years ago by
Replying to jmantysalo:
References to corrected bugss should be in
TESTS
section.
done
But more importantly, how this should work? If I understood correctly (have not compiled yet), then this will change current behaviour when
vertices2=None
. This can surprise users.
This is correct. Actually I did this patch because one of my colleague was surprized that the behavior of the method is different than what it claims to be. Since it has not been reported before, I suspect that very few people are using this method. I don't know if we have to follow a particular (and long) procedure for such case.
comment:6 in reply to: ↑ 5 Changed 6 years ago by
Replying to dcoudert:
Since it has not been reported before, I suspect that very few people are using this method. I don't know if we have to follow a particular (and long) procedure for such case.
We have deprecation policy, but it is hard to use in this case. Should we add a noteblock? Maybe not.
Thinking more about this... If I ask neighboring states of Finland and Sweden (they are neighbors), I expect the answer to be Norway, Denmark and Russia only. Using that as analogy, I think that this can go on.
I will compile and test this later today.
comment:7 Changed 6 years ago by
 Reviewers set to Jori Mäntysalo
 Status changed from needs_review to positive_review
Tests passed, documentation builds and is OK.
comment:8 Changed 6 years ago by
Thank you for the review. David.
comment:9 Changed 6 years ago by
 Branch changed from u/dcoudert/20479 to d35ce496637639f29294fa6c369f78caad401b46
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
trac #20479: correct behavior of vertex_boundary