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: sage-7.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:

Status badges

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 dcoudert

  • Branch set to u/dcoudert/20479
  • Commit set to fb0deec2f4556c1d2c63f872f4390db5ba678e8a
  • Status changed from new to needs_review

New commits:

fb0deectrac #20479: correct behavior of vertex_boundary

comment:2 Changed 6 years ago by dcoudert

  • Cc jmantysalo vdelecroix added

Simple patch that should be easy to review. Thanks.

comment:3 follow-up: Changed 6 years ago by jmantysalo

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 git

  • Commit changed from fb0deec2f4556c1d2c63f872f4390db5ba678e8a to d35ce496637639f29294fa6c369f78caad401b46

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

d35ce49trac #20479: add tests section

comment:5 in reply to: ↑ 3 ; follow-up: Changed 6 years ago by dcoudert

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 jmantysalo

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 note-block? 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 jmantysalo

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

Thank you for the review. David.

comment:9 Changed 6 years ago by vbraun

  • Branch changed from u/dcoudert/20479 to d35ce496637639f29294fa6c369f78caad401b46
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.