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

AllKNN using Dlib-ml #98

Merged
merged 7 commits into from Aug 14, 2017

Conversation

Projects
None yet
3 participants
@Iron-Stark
Contributor

Iron-Stark commented Aug 2, 2017

No description provided.

@rcurtin

Looks good; can you address the few comments I left please? I think it will be ready for merge shortly after that.

Show outdated Hide outdated Makefile
# Parse options into string.
optionsStr = ""
if "k" in options:
optionsStr = "-k " + str(options.pop("k"))

This comment has been minimized.

@rcurtin

rcurtin Aug 5, 2017

Member

k should definitely be required for k-nearest-neighbors search. Can you add an error here if the user does not include that option?

@rcurtin

rcurtin Aug 5, 2017

Member

k should definitely be required for k-nearest-neighbors search. Can you add an error here if the user does not include that option?

for (size_t i = 0; i < referenceData.n_cols; ++i)
{
for (size_t j = 0; j < referenceData.n_rows; ++j)
m(j) = referenceData(j, i);

This comment has been minimized.

@rcurtin

rcurtin Aug 5, 2017

Member

Can you use two spaces for indents please?

@rcurtin

rcurtin Aug 5, 2017

Member

Can you use two spaces for indents please?

Show outdated Hide outdated methods/dlibml/src/ALLKNN.cpp
Show outdated Hide outdated methods/dlibml/ALLKNN.py
Show outdated Hide outdated methods/dlibml/src/ALLKNN.cpp
Show outdated Hide outdated methods/dlibml/ALLKNN.py
@rcurtin

Looks good to me. Thanks for the hard work putting this together.

@Iron-Stark

This comment has been minimized.

Show comment
Hide comment
@Iron-Stark

Iron-Stark Aug 11, 2017

Contributor

@rcurtin

Thanks for appreciating the work :)

Contributor

Iron-Stark commented Aug 11, 2017

@rcurtin

Thanks for appreciating the work :)

@zoq zoq merged commit a8b1861 into mlpack:master Aug 14, 2017

1 check failed

Benchmarks Checks Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment