Skip to content
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

Patches: tasklogger log_x, randomized_svd arguments, deprecated graph_shortest_path #62

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

stanleyjs
Copy link
Collaborator

Good day,

This PR began its life to fix the issues with randomized svd discussed in #58. To fix that, I monkey patched in a version of randomized svd using mock that respects arguments passed in the dataclass bipca.base.PCAParameters. The patch works on all versions of sklearn, though it raises a warning to the user to let the maintainers of graphtools know that sklearn has been updated when the version exceeds the current version.
The PCAParameters dataclass has been documented and the pca_params kwarg to graphtools.base.Data has been documented.

I also made some small changes that made the tests run smoothly. I changed all calls to tasklogger.debug,.task,.info to their new log_x version. I changed instances of sklearn.utils.graph_shortest_path.graph_shortest_path to scipy.sparse.csgraph.shortest_path, reflecting the fact that the former has been deprecated.

The monkey patch required changing the way that SVD is done in the tests to make things line up properly. I fixed this in test_exact.test_truncated_exact_graph_sparse and test_knn.test_knn_graph_sparse.

This build currently fails two tests reliably, as documented in #60. This can be fixed pretty quickly using the suggested changes in that issue.
This build intermittently fails test_exact.test_shortest_path_affinity as discussed in #61. I believe master fails this as well.

@dburkhardt dburkhardt self-requested a review January 3, 2022 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant