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

Motifs tidy ii #1623

Merged
merged 17 commits into from
Jun 3, 2024
Merged

Motifs tidy ii #1623

merged 17 commits into from
Jun 3, 2024

Conversation

narnolddd
Copy link
Collaborator

What changes were proposed in this pull request?

small tidy up of variable names/removing dead code and also making a small optimisation
fix local motifs for windowed graphs.

Why are the changes needed?

Noticed I'd left some dead code in there. Optimisation was suggested a while ago by @LJeub which seems to shave off a few seconds in Alphabay but may be even better for bigger datasets.

Local motifs was throwing an error for nodes filtered out by window.

Does this PR introduce any user-facing change? If yes is this documented?

No

How was this patch tested?

Alphabay motifs produce same output in similar to slightly shorter time. Added windowed graph test for local motifs.

Issues

N/A

Are there any further changes required?

No

@narnolddd narnolddd requested a review from ljeub-pometry May 28, 2024 14:27
Copy link
Collaborator

@ljeub-pometry ljeub-pometry left a comment

Choose a reason for hiding this comment

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

Changes look good, note that you should be able to speed up the edge lookups by tracking internal instead of global node ids

@@ -445,6 +443,7 @@ mod motifs_test {
let actual = binding
.iter()
.map(|(k, v)| (k, v[0].clone()))
.into_iter()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not needed?

@narnolddd
Copy link
Collaborator Author

I unfortunately found what was secretly an n^2 routine in the local motif counting -- the algo was never seeming to finish on the large dataset and I found I was unintentionally calling neighbours about n^2 times. Changed now and it seems to be about the same time as the global motifs, just a little longer which is what I was expecting.

@miratepuffin miratepuffin merged commit 64cdce2 into master Jun 3, 2024
13 checks passed
@miratepuffin miratepuffin deleted the motifs-tidy-ii branch June 3, 2024 16:33
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.

3 participants