Skip to content

gds-replicating fastRP#141

Closed
AlfredClemedtson wants to merge 12 commits intoneo4j-labs:mainfrom
AlfredClemedtson:fast-rp
Closed

gds-replicating fastRP#141
AlfredClemedtson wants to merge 12 commits intoneo4j-labs:mainfrom
AlfredClemedtson:fast-rp

Conversation

@AlfredClemedtson
Copy link

No description provided.

}
}

struct RandomGenerator {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be good to document this beast to make it clear that it's essentially mirroring the Java RNG and that we need this to align with GDS behaviour.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe instead of documenting just that RNG, it would be better to document the whole algo and also link to the Java impl on GitHub.

pyo3-log = "0.7.0"
rand = "0.8.5"
rayon = "1.7.0"
rayon = "1.7.0, <1.10.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need this constraint?

Copy link
Author

Choose a reason for hiding this comment

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

Can't build with rayon 1.11. Looks like the builder crate needs <= 1.10.
error[E0271]: type mismatch resolving `<IntoIter<(NI, NI, EV)> as ParallelIterator>::Item == &_` --> crates/builder/src/input/edgelist.rs:125:35 | 125 | self.list.into_par_iter().copied() | ^^^^^^ expected `&_`, found `(NI, NI, EV)` |

self, *, chunk_size: int, neighbor_rounds: int, sampling_size: int
) -> WccResult:
"""Run Weakly Connected Components on this graph."""
def fast_rp(
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe also use the _gds suffix here to make this more clear?

Comment on lines +26 to +35
def test_coefficients(g: DiGraph):
pass


def test_normalization_strength(g: DiGraph):
pass


def test_random_seed(g: DiGraph):
pass
Copy link
Collaborator

Choose a reason for hiding this comment

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

mh?

Copy link
Collaborator

@s1ck s1ck left a comment

Choose a reason for hiding this comment

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

I like it and I am not against merging this and improving parallelism later. However, if it's not strictly necessary to mirror the exact GDS implementation, I'd prefer to do that instead.

Does mirroring GDS somehow limit you in terms of performance, e.g. the ability to parallelize more?

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.

2 participants