python-graphblas / graphblas-algorithms

Graph algorithms written in GraphBLAS
Apache License 2.0
76 stars 4 forks source link

Implement `google_matrix` and binary operators #62

Closed eriknw closed 1 year ago

eriknw commented 1 year ago

These aren't super-optimized or super-clean, but are hopefully "good enough" for a while.

eriknw commented 1 year ago

@jim22k, test_intersection is raising, and it looks like code you added. Can you help? Note that I implemented intersection in this PR.

jim22k commented 1 year ago

Other than my comment above, this all looks good to me.

eriknw commented 1 year ago

Other than my comment above, this all looks good to me.

Thanks for reviewing! Thoughts on the failing test?

jim22k commented 1 year ago

Thoughts on the failing test?

I think the line: if os.environ.get("NETWORKX_GRAPH_CONVERT", None) != "nx-loopback": should instead be: if os.environ.get("NETWORKX_GRAPH_CONVERT", None) is not None:

The point of that test is to check that nx._dispatch raises a TypeError when differently typed Graph objects are passed in. But we don't need to test that during auto dispatching tests.

Can you make that change in NetworkX and see if the tests pass? If so, please add that change to your NetworkX PR. Thanks!

eriknw commented 1 year ago

if os.environ.get("NETWORKX_GRAPH_CONVERT", None) is not None:

This doesn't work, b/c we convert networkx graphs (we always call e.g. to_graph). Should it instead be == "nx-loopback":? I still don't quite follow what's going on. Also, I think this ought to be broken out into its own test.

jim22k commented 1 year ago

Oops. Try this instead:

# Only test if not performing auto convert testing of backend implementations
if os.environ.get("NETWORKX_GRAPH_CONVERT", None) is None:

The goal is to only test this if we aren't doing auto convert testing.

eriknw commented 1 year ago

Merging. The test failure is understood--we are waiting on NetworkX PR to be merged.