ITensor / DataGraphs.jl

A simple graph type with data on the vertices and edges.
MIT License
12 stars 3 forks source link

Overload of NamedGraphs Functions for DataGraph #20

Closed JoeyT1994 closed 5 months ago

JoeyT1994 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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.