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

Support for vertex and hyperedge values #12

Merged
merged 5 commits into from
Feb 8, 2019
Merged

Conversation

pszufe
Copy link
Owner

@pszufe pszufe commented Feb 7, 2019

Support for vertex and hyperedge values following the discussion at issue #2
This also includes several new tests and a bug correction for function set_index!

src/hypergraph.jl Outdated Show resolved Hide resolved
src/hypergraph.jl Outdated Show resolved Hide resolved
src/hypergraph.jl Outdated Show resolved Hide resolved
@@ -72,8 +98,8 @@ Removes a vertex from a given hyperedge for a hypergraph `h` and a given vertex-
"""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so maybe add a comment that trying to remove a vertex from a hyperedge when it is not present is not an error, earlier we wanted it to throw an error.

src/hypergraph.jl Outdated Show resolved Hide resolved
src/hypergraph.jl Outdated Show resolved Hide resolved
src/hypergraph.jl Outdated Show resolved Hide resolved
src/hypergraph.jl Outdated Show resolved Hide resolved
src/hypergraph.jl Outdated Show resolved Hide resolved
src/hypergraph.jl Outdated Show resolved Hide resolved
src/hypergraph.jl Outdated Show resolved Hide resolved
src/hypergraph.jl Outdated Show resolved Hide resolved
Copy link
Collaborator

@bkamins bkamins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have left some comments.

A major missing part is load/save functionality (which needs a design of storage format in the first place).

Given it starts to become a more complex design maybe we should switch to TOML or JSON with storage?

@bkamins
Copy link
Collaborator

bkamins commented Feb 7, 2019

Also with metadata added probably we should consider saving the information about the types of a Hypergraph, as now it might start becoming relevant. This also probably means that we should implement a binary format (even based on a serialize/deserialize I think should be enough for practical purposes).

@pszufe
Copy link
Owner Author

pszufe commented Feb 8, 2019

According to my tests the best option for graph serialization is the BSON.jl.
On one hand BSON format provides cross-language compatibility and on the other hand this library has a good stability.

@pszufe
Copy link
Owner Author

pszufe commented Feb 8, 2019

On other note - since serialization is currenty being developed by the Salerno team - I definitely propose to handle that in a separate pull request.

src/hypergraph.jl Outdated Show resolved Hide resolved
@bkamins
Copy link
Collaborator

bkamins commented Feb 8, 2019

BSON.jl - OK. Serialization: who does work on serialization?

@pszufe
Copy link
Owner Author

pszufe commented Feb 8, 2019

I understand that @spagnuolocarmine and @aleant93 currently work on the hypergraph storage issues.

@bkamins
Copy link
Collaborator

bkamins commented Feb 8, 2019

This is independent (they are working on comments). We do not have do do it in this PR, but then please open an Issue to fix it later.

@bkamins bkamins deleted the pszufe-issue2-he_weights branch February 8, 2019 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants