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

Facebook sync (Jun 2019) #862

Merged
merged 22 commits into from
Jun 19, 2019
Merged

Conversation

beauby
Copy link
Contributor

@beauby beauby commented Jun 13, 2019

Changelog:

Bugfixes:

Features:

Misc:

  • throw python exception for OOM ( std::bad_alloc #758);
  • make DistanceComputer available for all random access indexes;
  • gradually moving from long to int64_t for portability.

Copy link
Contributor

@mdouze mdouze left a comment

Choose a reason for hiding this comment

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

Add in changelog

  • support for 6 new distance types in CPU index Flat and HNSW

  • refactoring to make DistanceComputer available for all random access indexes

@mdouze
Copy link
Contributor

mdouze commented Jun 13, 2019

Good to go IMO.

Copy link
Contributor

@Enet4 Enet4 left a comment

Choose a reason for hiding this comment

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

I could not compile this on my machine:

utils_simd.cpp: In function ‘float faiss::fvec_Linf(const float*, const float*, size_t)’:
utils_simd.cpp:484:12: error: ‘fvec_Linf_ref’ was not declared in this scope
     return fvec_Linf_ref (x, y, d);
            ^~~~~~~~~~~~~
utils_simd.cpp:484:12: note: suggested alternative: ‘fvec_L1_ref’
     return fvec_Linf_ref (x, y, d);
            ^~~~~~~~~~~~~
            fvec_L1_ref

A reference implementation is seen in utils_simd.cpp for fvec_L1_ref, but fvec_Linf_ref appears to be missing. I think something like this would do the trick:

float fvec_Linf_ref (const float * x,
                   const float * y,
                   size_t d)
{
    float res = 0;
    for (size_t i = 0; i < d; i++) {
        const float tmp = fabs(x[i] - y[i]);
        if (tmp > res) {
            res = tmp;
        }
    }
    return res;
}

As for int64_t not being found in some Travis jobs, it's likely missing a few #include <cstdint> in some places.

@mdouze
Copy link
Contributor

mdouze commented Jun 13, 2019

@Enet4 thanks for flagging. We forgot to test without the -mavx2 flag, which exposes this bug.

@beauby beauby force-pushed the sync20190613 branch 3 times, most recently from 9bad97d to 8d1aa5c Compare June 14, 2019 18:52
@beauby
Copy link
Contributor Author

beauby commented Jun 17, 2019

Only test left failing on OSX is a matter of system python not finding Homebrew-installed scipy.

@beauby beauby force-pushed the sync20190613 branch 2 times, most recently from 500ad58 to 11e5b09 Compare June 18, 2019 13:50
@beauby beauby merged commit 3896b12 into facebookresearch:master Jun 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants