Starting to fix #37 #40
Conversation
|
Hi, I did not have time yet to look to much into this PR, but it is highly appreciated. Will try to do that this week. The tests are a bit of a mess indeed, I will check where they would go best. |
| for s in neigs_v | ||
| rem_edge!(g, s, v) | ||
| end | ||
| values_selfloop = nothing # TODO: fix this type instability |
There was a problem hiding this comment.
I would not worry too much about type instability. If you have some variable of type Union{T, Nothing} for some type T, then Julia will still be able to create efficient code. In general, Julia can generate efficient code, as long as there are not too many types involved in an Union. That's why using @code_warntype will also color this variable yellow and not red.
| # remove the in_edges incident to v | ||
| neigs_v = copy(inneighbors(g, v)) | ||
| for s in neigs_v | ||
| rem_edge!(g, s, v) |
There was a problem hiding this comment.
I wonder if it were not better, if we would work directly with the adjacency lists here - after all this method does not have to solve the abstract case, but only has to work for this graph.
|
So I would suggest you add tests to |
Hi there 👋
I tried to code a first version of
rem_vertex!by drawing inspiration from the one inLightGraphs.jl. It only works onValGraphfor now, because I wanted to discuss the implementation.I didn't add tests because they seem to be broken for this part of the codebase (which is probably why this line is commented?).
Anyway, let me now what you think!