Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add_vertex! API #41

Closed
filchristou opened this issue Jan 27, 2023 · 6 comments
Closed

add_vertex! API #41

filchristou opened this issue Jan 27, 2023 · 6 comments
Assignees
Labels
enhancement New feature or request

Comments

@filchristou
Copy link
Contributor

Some discussions (JuliaGraphs/Graphs.jl#122, JuliaGraphs/Graphs.jl#146, JuliaGraphs/Graphs.jl#165) argue that add_vertex! should return the index of the newly added vertex. Currently, the compromise is that add_vertex! returns Bool.

Would you consider entering a transient phase and support both ?

For example, this would mean appropriately modifying the following line:


A valid alternative would be to check nv(meta_graph.graph) before and after the add_vertex! operation.

Motivation

As mentioned in #40 and QuantumBFS/Multigraphs.jl#17 I would like to generalize the use cases for MetaGraphsNext and more specifically have it work with Multigraphs.
Multigraphs.jl's add_vertex! already return an index.

@bramtayl
Copy link
Collaborator

bramtayl commented Feb 7, 2023

I'm indifferent here. If I had designed it, I'd have it return nothing, because users can also just use nv to find the new number of vertices, and error if the vertex wasn't added. But if others find it useful, sure, we can return the new index.

@filchristou
Copy link
Contributor Author

MetaGraphsNext.jl is actually a wrapper, so I wouldn't try to impose a new style. add_vertex!(mg::MetaGraphsNext.MetaGraph) should return whatever add_vertex!(mg.graph) returns. The best we can do, is at least handle the API internally in a neutral fashion. Instead of doing the check if add_vertex!(mg.graph) do the check

nvnum = nv(mg.graph)
add_vertex!(mg.graph)
if nv(mg.graph) != nvnum ...

If you agree with this changes, let me know and I can start working on a PR to substitute such checks.

@bramtayl
Copy link
Collaborator

bramtayl commented Feb 9, 2023

Sure that sounds good thanks!

@gdalle gdalle added the enhancement New feature or request label Feb 22, 2023
@gdalle
Copy link
Member

gdalle commented Feb 22, 2023

If you agree with this changes, let me know and I can start working on a PR to substitute such checks.

I also agree with this :)

@filchristou
Copy link
Contributor Author

can you please assign this issue on me, so that I don't forget this ?

filchristou added a commit to filchristou/MetaGraphsNext.jl that referenced this issue Feb 28, 2023
@gdalle
Copy link
Member

gdalle commented Mar 2, 2023

Fixed by #47

@gdalle gdalle closed this as completed Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants