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

Added trials parameter to leiden #790

Merged
merged 1 commit into from May 19, 2021
Merged

Added trials parameter to leiden #790

merged 1 commit into from May 19, 2021

Conversation

daxpryce
Copy link
Contributor

@daxpryce daxpryce commented May 18, 2021

Added trials parameter to leiden and set a new requirement of graspologic-native 1.0.0 or higher to use it.

  • Does this PR add any new dependencies?
  • Does this PR modify any existing APIs?
    • Is the change to the API backwards compatible?
  • Have you built the documentation (reference and/or tutorial) and verified the generated documentation is appropriate?

@netlify
Copy link

netlify bot commented May 18, 2021

Deploy Preview for graspologic failed.

Built with commit 0f6a1a9

https://app.netlify.com/sites/graspologic/deploys/60a43eb4aeb92300078cea94

Comment on lines +328 to +331
if not isinstance(trials, int):
raise TypeError("trials must be a positive integer")
if trials < 1:
raise ValueError("trials must be a positive integer")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we've talked about input checking at various times - I've always thought it could be helpful to use things like https://scikit-learn.org/stable/modules/generated/sklearn.utils.check_scalar.html going forward, just to be more concise and consistent. I don't care for the purposes of this PR but thought I'd bring it up.

@@ -277,6 +278,13 @@ def leiden(
if it is found to be a directed graph. If you know it is undirected and wish to
avoid this scan, you can set this value to ``False`` and only the lower triangle
of the adjacency matrix will be used to generate the weighted edge list.
trials : int
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't want to let this hold up the PR, but this seems like something we should add to the stack of #755 and consider how we name parameters like this in the future. This exact thing happens in match and cluster at a minimum and there we call it n_init.

@bdpedigo bdpedigo self-requested a review May 19, 2021 14:00
@bdpedigo
Copy link
Collaborator

for some reason netlify build failed with

6:26:01 PM: ERROR: Could not find a version that satisfies the requirement graspologic-native>=1.0.0 (from graspologic[dev])
6:26:01 PM: ERROR: No matching distribution found for graspologic-native>=1.0.0

@bdpedigo bdpedigo merged commit 1321406 into dev May 19, 2021
@bdpedigo bdpedigo deleted the leiden-trials branch May 19, 2021 16:21
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.

None yet

3 participants