Fix infinite method call in forest_cover_edge_sequence; add tests.

[jack-dunham]

The forest_cover_edge_sequence method dispatching on AbstractNamedGraph would call itself repeatedly as there was no forest_cover_edge_sequence method dispatching on concrete NamedGraph.

I've removed this method as it was also not necessarily given the generic implementation of forest_cover_edge_sequence on AbstractGraph, and added some simple tests for this function.

This PR also refactors the implementation of similar_graph, moving this function back into GraphsExtensions.

[codecov]

Codecov Report

❌ Patch coverage is 90.14085% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.57%. Comparing base (7284411) to head (e1ab8b3).

Files with missing lines	Patch %	Lines
src/similar_graph.jl	80.00%	4 Missing ⚠️
src/lib/GraphsExtensions/src/abstractgraph.jl	91.66%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #170      +/-   ##
==========================================
+ Coverage   78.99%   80.57%   +1.58%     
==========================================
  Files          55       56       +1     
  Lines        2099     2111      +12     
==========================================
+ Hits         1658     1701      +43     
+ Misses        441      410      -31     

Flag	Coverage Δ
docs	0.00% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[mtfishman]

Looks good, ready to merge? Would this mean we don't need to define forest_cover_edge_sequence for AbstractDataGraph, i.e. in ITensor/DataGraphs.jl#121?

[mtfishman]

Looks like some downstream tests are failing, is the goal to make this PR non-breaking?

[jack-dunham]

The ITensorNetworksNext tests failing is strange, i'll investigate.

I am less bothered if DataGraphs breaks and needs updated slightly as we have an open PR for that package anyway, so can upgrade to the new NamedGraphs version (assuming that doesn't breaking downstream tests).

[mtfishman]

The ITensorNetworksNext tests failing is strange, i'll investigate.

I am less bothered if DataGraphs breaks and needs updated slightly as we have an open PR for that package anyway, so can upgrade to the new NamedGraphs version (assuming that doesn't breaking downstream tests).

I agree it isn't a huge deal if this PR is breaking, I just wanted to check if that is deliberate, check how easy it would be to make it non-breaking, inquire what specifically about the PR is breaking, etc. But also if it does break DataGraphs we should mark this PR as breaking with a minor version bump.

[jack-dunham]

inquire what specifically about the PR is breaking

For DataGraphs, there's a test of the function empty_graph on a DataGraph (checks this function removes data). I have moved this function from NamedGraphs to GraphsExtensions, as is generic to any AbstractGraph, so the import statement in the tests fails.

That should be the only place where code is breaking AFAIK. We can reverse this change, but it seems logical to move that function in this PR.

[mtfishman]

inquire what specifically about the PR is breaking

For DataGraphs, there's a test of the function empty_graph on a DataGraph (checks this function removes data). I have moved this function from NamedGraphs to GraphsExtensions, as is generic to any AbstractGraph, so the import statement in the tests fails.

That should be the only place where code is breaking AFAIK. We can reverse this change, but it seems logical to move that function in this PR.

Can we add a statement using .GraphsExtensions: empty_graph in NamedGraphs for backwards compatibility?

[mtfishman]

Probably doesn't matter too much, but I don't see what this copy is needed for since it doesn't look like the graph is being modified in-place. If it isn't needed we can just write g_reduced = g if the point is primarily to have a different variable name to use. I would just find that clearer since when I saw a copy I then expected to see an in-place operation somewhere.

[jack-dunham]

Good point.