search

JuliaGraphs / MetaGraphsNext.jl

A package for graphs with vertex labels and metadata in Julia
http://juliagraphs.org/MetaGraphsNext.jl/
Other
73 stars 17 forks source link

Fixed updating vertex properties upon removal #34

Closed filchristou closed 2 years ago

filchristou commented 2 years ago

A simple fix to #33

filchristou commented 2 years ago

So what is really happening is when a vertex is getting deleted, actually the the last vertex is copied in its place and the last vertex is deleted after. Obviously the lines in https://github.com/JuliaGraphs/MetaGraphsNext.jl/blob/485c256dea238a546a80701ff5a345edb347b936/src/graphs.jl#L129

            lastvprop = vertex_properties[lastl]
...
            vertex_properties[lastl] = lastvprop

were buggy and I think just fixing the last one with the correct index solves the problem.

filchristou commented 2 years ago

fyi. if you want to inspect my branch do so with:

julia> Pkg.add(url="https://github.com/filchristou/MetaGraphsNext.jl.git", rev="rem_vertexSAFE#33")

https://github.com/JuliaLang/Pkg.jl/issues/3157

bramtayl commented 2 years ago

Sorry I've been a bit busy. Thanks for the PR! I'll try to look at this this weekend.

bramtayl commented 2 years ago

This looks good! Can you add a test that would have caught https://github.com/JuliaGraphs/MetaGraphsNext.jl/issues/33?

filchristou commented 2 years ago

Sure but I am not sure where. ./test/* is empty. Do you want me to create a new file in ./test/ or just add something in the ./docs e.g. in Tutorial=>Basics ? I find it more appropriate to be in ./test/ since the behavior is already described in the tutorial.

bramtayl commented 2 years ago

Sure, you can add it to the end of test/runtests.jl if you want.

bramtayl commented 2 years ago

The test failure is my fault. I was really excited about doctests but because printing changes in julia versions they aren't very robust. So we'll need to copy all the doctests as regular tests and only run the doctests on one julia version.

filchristou commented 2 years ago

So we'll need to copy all the doctests as regular tests and only run the doctests on one julia version.

I would suggest a new issue for this.

bramtayl commented 2 years ago

@filchristou can you rebase against master?

bramtayl commented 2 years ago

Fixed in 0.4.0. Thanks!