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

Move the extend algorithm into the C++ extension #178

Merged
merged 18 commits into from
Aug 4, 2021
Merged

Conversation

johnlees
Copy link
Member

@johnlees johnlees commented Aug 2, 2021

This moves LineageFit.extend to the poppunk_refine extension. The algorithm is similar to that used in sparsifyDists in pp_sketchlib, but works specifically with a combination of sparse and dense matrices. The highest rank is computed, and then lowered to get smaller ranks. Parallelisation supported.

@johnlees
Copy link
Member Author

johnlees commented Aug 2, 2021

Timing of extend on 10k ref vs 10k query:
1 thread: 19s
2 threads: 13s
4 threads: 8s
8 threads: 4s

@johnlees johnlees marked this pull request as ready for review August 2, 2021 16:52
@johnlees johnlees requested a review from nickjcroucher August 2, 2021 16:55
bool self = true, const int int_offset = 0) {
edge_tuple edges =
generate_all_tuples(num_ref, num_queries, self, int_offset);
return (edges);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

You just hate whitespace, don't you?

Copy link
Member Author

Choose a reason for hiding this comment

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

You need to install the best vs code extension around: https://marketplace.visualstudio.com/items?itemName=shardulm94.trailing-spaces

Copy link
Member Author

Choose a reason for hiding this comment

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

And then once you've accepted that into your life, you can get really pedantic: https://marketplace.visualstudio.com/items?itemName=oderwat.indent-rainbow

Copy link
Member Author

@johnlees johnlees Aug 4, 2021

Choose a reason for hiding this comment

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

Although actually this particular change was courtesy of: https://marketplace.visualstudio.com/items?itemName=xaver.clang-format

Copy link
Member Author

Choose a reason for hiding this comment

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

(I have a lot of extensions)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would inevitably manage to install two extensions with conflicting preferences that would infinite loop as soon as I indented a line

Copy link
Collaborator

@nickjcroucher nickjcroucher left a comment

Choose a reason for hiding this comment

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

Only one small comment - might be slightly clearer to have new_val be true when there is a substantial increment in the distance, but I appreciate it is of limited importance to the mechanics of the functions where it appears.

@johnlees
Copy link
Member Author

johnlees commented Aug 4, 2021

Only one small comment - might be slightly clearer to have new_val be true when there is a substantial increment in the distance, but I appreciate it is of limited importance to the mechanics of the functions where it appears.

This is a good point, and though it works correctly, I've already confused myself once because the name suggests the opposite meaning to what it should be

Copy link
Collaborator

@nickjcroucher nickjcroucher left a comment

Choose a reason for hiding this comment

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

Brillant, looks great, works very nicely!

@johnlees johnlees merged commit f748a59 into master Aug 4, 2021
@johnlees johnlees deleted the extend_cpp branch August 4, 2021 10:45
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