LightGraphs.jl icon indicating copy to clipboard operation
LightGraphs.jl copied to clipboard

fix induced_subgraph indexing with vector of Bools

Open bkamins opened this issue 3 years ago • 4 comments

Currently both getindex and induced_subgraph handle AbstractVector{Bool} argument in a way inconsistent with Julia Base rules.

bkamins avatar Jul 05 '21 16:07 bkamins

Codecov Report

Merging #1573 (37c6aca) into master (712b8d1) will increase coverage by 0.00%. The diff coverage is 100.00%.

:exclamation: Current head 37c6aca differs from pull request most recent head 5032b51. Consider uploading reports for the commit 5032b51 to get more accurate results

@@           Coverage Diff           @@
##           master    #1573   +/-   ##
=======================================
  Coverage   99.44%   99.44%           
=======================================
  Files         106      106           
  Lines        5551     5554    +3     
=======================================
+ Hits         5520     5523    +3     
  Misses         31       31           

codecov[bot] avatar Jul 05 '21 16:07 codecov[bot]

Sorry - what's the specific problem here?

sbromberger avatar Jul 05 '21 19:07 sbromberger

It seems as if you're trying to use a bitvec / vector of boolean where true values indicate inclusion in the induced subgraph. That's not (currently) how it works: the vlist is a list of vertex indices.

sbromberger avatar Jul 05 '21 19:07 sbromberger

I agree it does not work this way now. That is why I propose to change it (indeed - docstrings should be updated then; I can fix it if you agree to the proposal).

  1. I think what I propose would be often useful (when you have a predicate that you apply to a vertex to decide if it should stay or not stay)
  2. you cannot use Bool as index now anyway (so the change is not breaking), see:
julia> g = SimpleGraph(2)
{2, 0} undirected simple Int64 graph

julia> g[[true]]
ERROR: ArgumentError: invalid index: true of type Bool

julia> g[[true, true]]
ERROR: ArgumentError: Vertices in subgraph list must be unique

  1. You call this function in getindex and current getindex definition is inconsistent with the definition in Julia Base as commented in the original comment. Consider the following examples:
julia> g = SimpleGraph(2)
{2, 0} undirected simple Int64 graph

julia> g[[true, false]]
ERROR: InexactError: Bool(2)

julia> x = [1, 2]
2-element Vector{Int64}:
 1
 2

julia> x[[true, false]]
1-element Vector{Int64}:
 1

bkamins avatar Jul 05 '21 21:07 bkamins