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

Overload of NamedGraphs Functions for DataGraph #20

Merged
merged 1 commit into from
Jan 22, 2024

Conversation

JoeyT1994
Copy link
Contributor

@JoeyT1994 JoeyT1994 commented Jan 19, 2024

This is Quick PR to allow a few functions from NamedGraphs.jl to work on a DataGraph.

Specifically these changes allow the functions parent_graph(dg::DataGraph) and parent_vertices_to_vertices(dg::DataGraph, args...) to work by passing to the underlying_graph. This is necessary for upcoming changes to ITensorNetworks.jl and I would prefer to do it more generally here than on just the AbstractITensorNetwork type.

There is an implicit assumption here that underlying_graph(dg) is an AbstractNamedGraph as opposed to say a SimpleGraph (for which it will error). @mtfishman I could add an explicit error check for this if you wish? But this is also our assumption going forward in a lot of ITensorNetworks.jl code: i.e. that we always work with AbstractNamedGraphs and that certain functionality will not work if the underlying_graph is a SimpleGraph instead.

@JoeyT1994
Copy link
Contributor Author

JoeyT1994 commented Jan 19, 2024

Note that this is related to Issue #19 and if we did make DataGraph a subtype of AbstractNamedGraph it would avoid needing to overload functions like this.

Alternatively we just enforce the restriction that underlying_graph is an AbstractNamedGraph in the definition of a DataGraph, i.e.

struct DataGraph{V,VD,ED,G<:AbstractNamedGraph,E<:AbstractEdge} <: AbstractDataGraph{V,VD,ED}
  underlying_graph::G
  vertex_data::Dictionary{V,VD}
  edge_data::Dictionary{E,ED}
  function DataGraph{V,VD,ED,G,E}(
    underlying_graph::G, vertex_data::Dictionary{V,VD}, edge_data::Dictionary{E,ED}
  ) where {V,VD,ED,G<:AbstractNamedGraph,E<:AbstractEdge}
    @assert vertextype(underlying_graph) == V
    @assert edgetype(underlying_graph) == E
    return new{V,VD,ED,G,E}(underlying_graph, vertex_data, edge_data)
  end
end

@mtfishman
Copy link
Member

I think the current PR is good, I would rather not be too restrictive about type constraints just yet. We probably need to assess if DataGraphs code really works when the underlying graph is a SimpleGraph (my guess is that it doesn't, since I don't think it properly handles updating the keys of the edge/vertex data if vertices of a SimpleGraph are removed), but the current design allows for the possibility that a DataGraph wraps something that acts like a NamedGraph but isn't a AbstractNamedGraph subtype. Generally I'm trying to think about designing code more around interfaces rather than type hierarchies, so the current code is more aligned with that goal.

@JoeyT1994
Copy link
Contributor Author

Okay that sounds sensible. I think it probably doesn't work when the underlying graph is a SimpleGraph although maybe some basic functions still work. Nonetheless it should be clear to us when it errors and we are not really using the SimpleGraph type in the ITensorNetworks.jl source code or any examples.

@mtfishman mtfishman merged commit eacadeb into ITensor:main Jan 22, 2024
9 checks passed
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