ITensor / NamedGraphs.jl

Extension of `Graphs.jl` to graphs with named vertices.
MIT License
6 stars 3 forks source link

Better functionality for PartitionedGraph #45

Closed JoeyT1994 closed 9 months ago

JoeyT1994 commented 9 months ago

This PR adds improved functionality and function naming for the PartitionedGraph type.

Firstly, we add support for reverse(pe::PartitionedEdge) which previously required the user to unwrap and wrap the edge again.

Secondly, there is proper support for getting edges out of a PartitionedGraph. New functions in this domain:

These functions are exported.

mtfishman commented 9 months ago

Thanks.

I prefer partition_edge/partition_edges over which_partitionedge/which_partitionedges and partition_vertex/partition_vertices over which_partition/which_partitions.

I agree with renaming partition_edges to just edges.

I would call them partition_edges(pg)/partition_vertices(pg) instead of partitionedges(pg)/partitionvertices(pg), since we generally are using snake casing for other functions with similar names in this package.

JoeyT1994 commented 9 months ago

Yeah I am happy with that naming. The only issue is that we also have partition_vertices(pg; kwargs...) defined for when calling a partitioning backend like KaHyPar. Any thoughts on what we should rename that function to avoid the clash?

Maybe we could just call it partition(pg; partitioning_kwargs...) ?

mtfishman commented 9 months ago

I see, yeah that is an unfortunate name clash. I don't like partition(pg) for that since it doesn't indicate that it is outputting sets/groups of vertices (I would think that it might be outputting a partitioned graph, and in fact we might want to make that a simpler API for constructing a PartitionedGraph).

The problem seems to basically be that when reading partition_vertices(...), partition could be interpreted either as a verb (as in, the function is performing a partitioning of the vertices), or as a noun (as in, the vertices of a partitioned graph). It feels more natural to me to interpret it as a verb, and I think for that reason the names PartitionVertex and PartitionEdge have felt a bit awkward to me, but I can't think of better names for those so we are left with this conundrum.

One way out could be to call the function that partitions the vertices with KaHyPar/Metis partitioned_vertices(pg; kwargs...), to name it after what it is outputting rather than what it is doing (as in, that function is outputting sets/groups of partitioned vertices).

JoeyT1994 commented 9 months ago

Okay will go with partitioned_vertices(pg; kwargs...) for now

codecov-commenter commented 9 months ago

Codecov Report

Attention: 22 lines in your changes are missing coverage. Please review.

Comparison is base (40f0edd) 78.83% compared to head (941b4d2) 77.70%.

Files Patch % Lines
src/Graphs/partitionedgraphs/partitionedgraph.jl 20.00% 12 Missing :warning:
...aphs/partitionedgraphs/abstractpartitionedgraph.jl 22.22% 7 Missing :warning:
src/Graphs/partitionedgraphs/partitionedge.jl 0.00% 2 Missing :warning:
.../Graphs/partitionedgraphs/abstractpartitionedge.jl 0.00% 1 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #45 +/- ## ========================================== - Coverage 78.83% 77.70% -1.14% ========================================== Files 31 31 Lines 1167 1184 +17 ========================================== Hits 920 920 - Misses 247 264 +17 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mtfishman commented 9 months ago

Looks good, thanks. Could you add tests for the new functions?

mtfishman commented 9 months ago

Something I realized is that I think the snake casing in partition_vertex/partition_vertices and partition_edge/partition_edges may not be the right way to go. My thinking is that, with the current design, we are establishing a convention that partition_vertex(...) will output a PartitionVertex, partition_vertices will output a list of PartitionVertex, etc., which I think is a good convention to establish and makes the code clearer (and makes it clearer to us what to name functions going forward).

I think removing the snake casing, i.e. changing to partitionvertex, partitionvertices, etc., could make that convention even more obvious. To me, reading those new names makes it clearer that there is a connection between the name of the function and the type of object it is outputting (i.e., it makes it clearer that "partition" in the function name partitionvertex is a noun rather than a verb, and will output an object of type PartitionVertex).

I'm on the fence about that since we use snake casing in other functions, let me know what you think.

JoeyT1994 commented 9 months ago

I think partitionvertex (as opposed to partition_vertex ) is clearer to me in terms of what it is outputting (i.e. a PartitionVertex) and so I am in favor of that to avoid ambiguity about partition being a verb here. In fact here we could argue that we are not even necessarily forgoing snake casing as we have defined a PartitionVertex type and so partitionvertex is just simply a one word noun and no spaces are required?

mtfishman commented 9 months ago

Yeah, I think that is a more succinct reason for using the style partitionvertex. Also, it is common practice in Julia to have two sets of constructors for types, one using camel casing and one using lower casing (for example Tuple/tuple and String/string) which have different behaviors. Often the camel cased constructor is lower level and stricter. So this is a good example of following that convention.

JoeyT1994 commented 9 months ago

Okay great. I have gone with that naming convention for now! I also added further testing.

mtfishman commented 9 months ago

Looks good, thanks!