-
Notifications
You must be signed in to change notification settings - Fork 4
[PartitionedGraphs] Rename various types and functions to improve clarity. #110
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
base: main
Are you sure you want to change the base?
Conversation
|
Considering this is technically a breaking change I've opted to increment the minor version number. |
|
Thanks @jack-dunham, I think these names are clearer. @JoeyT1994 let us know what you think of these changes, I think we've discussed these name changes. @jack-dunham when you make breaking changes you need to bump the subdirectory Project.toml compat entries accordingly (i.e. |
|
Yeah I like these name changes -> they are more intuitive and quotient graph is in-line with the definition from the graph-theory community. |
|
@jack-dunham @JoeyT1994 I'll merge this so we can start using it in ITensorNetworksNext.jl. ITensorNetworks.jl and TensorNetworkQuantumSimulator.jl will need to be upgraded but that should be pretty easy since it is just a name change. |
|
|
||
| #Needed for interface | ||
| partitions_graph(pg::AbstractPartitionedGraph) = not_implemented() | ||
| quotient_graph(pg::AbstractPartitionedGraph) = not_implemented() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a slight nitpick, but could you rename this to quotientgraph? My reasoning is that generally this will construct a QuotientGraph, so we can think of it as the lowercase function version of QuotientGraph.
In fact to simplify things right now maybe we could just use QuotientGraph directly and get rid of the function version... I think we introduced that function since PartitionsGraphView was a long name to write out, but QuotientGraph is easy to write. @jack-dunham @JoeyT1994 curious to hear your thoughts on that (best to decide now while we are making a breaking change...).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the function is unnecessary especially as QuotientGraph is intended a wrapper type. I also think using the constructor directly is clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, let's go with that, we can always add a function back if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to add a (very simple) inner constructor to avoid recursion, so now QuotientGraph must wrap an AbstractPartitionedGraph. Passing an AbstractGraph that does not subtype AbstractPartitionedGraph still passes the trivial partition of this graph to QuotientGraph, as before.
All tests pass on my end.
|
Your PR no longer requires formatting changes. Thank you for your contribution! |
| struct PartitionsGraphView{V, G <: AbstractGraph{V}} <: AbstractNamedGraph{V} | ||
| using Graphs: AbstractGraph | ||
|
|
||
| struct QuotientGraph{V, G <: AbstractGraph{V}} <: AbstractNamedGraph{V} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we want:
| struct QuotientGraph{V, G <: AbstractGraph{V}} <: AbstractNamedGraph{V} | |
| struct QuotientGraph{V, G <: AbstractPartitionedGraph{V}} <: AbstractNamedGraph{V} |
to make this even more explicit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that relates to our discussion around representing a partitioned tensor network, i.e. a type can act like a partitioned graph without explicitly being an AbstractPartitionedGraph subtype, as long as it overloads a minimal interface of functionality related to how the graph is partitioned. So I think it is better to keep it more general and think of it more as an interface requirement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, in that case we need to define that interface. I suggest we remove the method:
QuotientGraph(g::AbstractGraph) = QuotientGraph(PartitionedGraph(g, [vertices(g)]))It doesn't seem required for any tests to pass, and define a new function partitions_graph:
partitions_graph(g::PartitionedGraph) = pg.partitions_graphSo a TensorNetwork with underlying_graph of type PartitionedGraph needs to define a method for partitions_graph, i.e.
partitions_graph(tn::TensorNetwork) = partitions_graph(underlying_graph(tn))Perhaps this can be defined on AbstractDataGraph? Although I feel like that is getting a little cumbersome. Perhaps it is best to just make the choice to wrap TensorNetwork in PartitionedGraph for now? That way one can just repeatedly define more and more layers of partitioning, including partitioning the quotient graphs of any of these partitions, with the tensor network data always sitting at the base of this.
This PR renames some types and functions in the
PartitionedGraphslibrary for clarity. Specifically:PartitionsGraphView->QuotientGraph.PartitionVertex->SuperVertex.PartitionEdge->SuperEdge.The associated functions pertaining to these types have also been renamed accordingly. I have also attempted to rename local variables to reflect this change.
Tests have been updated, and by some miracle, all passed first time.