Vertex number increases when illegally defining new node in test_throws

[wouterwln]

In the following piece of code:

using Graphs
using MetaGraphsNext
using Test

graph =  MetaGraph(
        Graph(),
        Label = Symbol,
        VertexData = Symbol)

println(nv(graph))

@test_throws MethodError graph["illegal"] = "not a symbol"
println(nv(graph)) 

We are testing if an illegal addition of a node throws the appropriate MethodError. However, the output here will be:

0
1

Which is somewhat unexpected: You'd expect the number of vertices to not increase when testing code like this (assert that an illegal node creation keeps the number of vertices the same)

[gdalle]

Good catch! I had already considered making dispatch much stricter on functions like add_vertex!, basically requiring that the label and data should have the right type. This would prevent such behavior, but it would also restrict things that are possible through conversion

[gdalle]

At the moment I struggle to see how to prevent this without a try/catch or stricter dispatch. @bramtayl any insights?

[wouterwln]

@gdalle is there a specific reason you call Graphs.add_vertex! before adding the data? Otherwise you could make sure the MethodError is thrown before adding a vertex to the underlying Graph. In other words: when does add_vertex(::Graph) fail? I'm not familiar enough with the Graphs.jl package to understand the implications of this change

[wouterwln]

Second idea, although suboptimal. You could assert that keytype(meta_graph.vertex_properties) == typeof(label) Although this would throw you an AssertionError instead of the (I think desired) MethodError

[gdalle]

In other words: when does add_vertex(::Graph) fail?

It very much depends on the underlying graph. You could have graphs with a fixed number of vertices for instance

[gdalle]

@gdalle is there a specific reason you call Graphs.add_vertex! before adding the data?

No it's arbitrary. But the good thing is that with add_vertex! I can check if it worked after the fact, without trying and catching an exception.

You could assert that keytype(meta_graph.vertex_properties) == typeof(label)

I guess this is one way of checking before inserting into the dictionary, but is it the only way in which a dict insertion can fail?

[bramtayl]

I don't think there's a great way to solve this. We could add another function (safe_add_vertex!?) which does try-catch. I don't think it's a great idea to do it by default though because of the performance impacts.

keytype(meta_graph.vertex_properties) == typeof(label)

This seems a little too strict. Julia does some conversions by default:

julia> x = Dict{String, Int}()
Dict{String, Int64}()

julia> x[SubString("hello world", 1:5)] = 0
0

julia> x["hello"]
0

[gdalle]

I think I found a fix that doesn't require try/catch, wanna review?