Return correct type add_vertex! and add_vertices!

[filchristou]

add_vertex! is supposed to return a Bool and add_vertices! an integer notating the number of vertices that were added.
Usually add_vertices uses iteratively add_vertex, but this package deals with this the other way around.
really, why do this ? The way it's done now, add_vertices doesn't also check for overflows.

I stumbled into this problem because I tried playing with MetaGraphsNext.MetaGraph{Multigraph} and it hit an error here: https://github.com/JuliaGraphs/MetaGraphsNext.jl/blob/71cf31d8710fa419896af3ae03b9d4f943844402/src/graphs.jl#L117

[codecov]

Codecov Report

Base: 92.46% // Head: 91.90% // Decreases project coverage by -0.56% ⚠️

Coverage data is based on head (fee9930) compared to base (f7553c7).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #17      +/-   ##
==========================================
- Coverage   92.46%   91.90%   -0.56%     
==========================================
  Files           5        6       +1     
  Lines         345      346       +1     
==========================================
- Hits          319      318       -1     
- Misses         26       28       +2     

Impacted Files	Coverage Δ
src/abstract_multigraph.jl	64.51% <100.00%> (ø)
src/multigraph_adjlist.jl	93.10% <100.00%> (-0.87%)	⬇️
src/di_multigraph_adjlist.jl	94.28% <0.00%> (-0.72%)	⬇️
src/Multigraphs.jl	100.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[ChenZhao44]

Thanks for the issue.

This design of returning indices when adding new vertex in Multigraphs.jl is for keep track on the new indices. As Multigraphs.jl is originally designed for ZXCalculus.jl where we need to manipulate graphs as internal data structures, returning all indices instead of returning the number of vertices is much more useful.

I am not willing to change this convention because there exists some packages depending on Multigraphs.jl. There still other ways to get rid of this issue. For example, you could overload that function with a specified type like

Graphs.add_vertex!(meta_graph::MetaGraph{Multigraph}, label, data)

Please feel free to ask me any questions about this package.

[filchristou]

Thank you for giving the time to look at this PR. I see.
However I think that it would be nice to focus on the long term prosperity and consistency of the julia graph ecosystem.
Making such choices means that your package will not be able to play well with the rest of the community.

For example, you could overload that function with a specified type like...

Apart from the fact that this will be type piracy, I think the authors of MetaGraphsNext.jl will not be happy to add an extra dependency to this package.
Maybe this can change with the introduction of weak dependencies, but, again, it's bad practice, so I doubt it.

Seeing the dependent packages, it appears that you are involved in them, i.e. you could potentially apply the changes yourself.
https://juliahub.com/ui/Packages/Multigraphs/0vrTA/0.3.0?page=2

So forgive me but I would like to ask again, and since I know this change is breaking, please take your time with this.
I believe it's in the best interest of everybody to encourage a common interface.

[ChenZhao44]

There is no need to add dependency of Multigraphs in MetaGraphsNext. Both of these packages are paralleled packages based on the Graphs API. I think the correct place to do the function overloading patch is in your own projects or packages. Would you like to discuss about the use cases of having meta-multigraph?

The data structure design in Graphs and Multigraphs are quite different. For example, SimpleGraph uses Array to store the adjacent list while Multigraph uses dictionary. Therefore, removing vertices will change indices in SimpleGraph but not for the Multigraph. Those different cause different API return types.

Back to 2020 when we started developing this package, the API suggestion was still not mature. But now the API convention of Graphs is better. But I still did not find the API suggestion about add_vertex! in Graphs. Could you send me the link if you find anything about it?

[filchristou]

Would you like to discuss about the use cases of having meta-multigraph?

Nothing extravagant. I just need a multigraph structure with each edge having some properties and I don't want to carry around an extra data structure.
More specifically, the motivation is an implementation of the algorithm in this paper.

Could you send me the link if you find anything about it?

Up until now, I was relying on the add_vertex! docs and the implementation.

I searched a little bit further in some discussions (JuliaGraphs/Graphs.jl#122, JuliaGraphs/Graphs.jl#146, JuliaGraphs/Graphs.jl#165) and it appears that there are motives to change this. They propose to return the index of the added vertex as an alternative.

I suppose in this sense, your package is okey. Maybe add a [] to the return a value and not a Vector in

Multigraphs.jl/src/abstract_multigraph.jl

Line 129 in 6ec2766

add_vertex!(mg::AbstractMultigraph{T}) where {T<:Integer} = add_vertices!(mg, one(T))

, but for now that's a detail.

Now the struggle is the MetaGraphsNext.jl is relying on the current API and Multigraphs.jl is sort of relying in the forthcoming.
Not an easy situation..

[ChenZhao44]

Thanks for the discussion. It seems still too early to change the design. Let us keep track on the Graphs 2.0 API standard.

For your current usage, a patch might be the simplest way to intergrate meta-multigraphs.