LshIndexParams broken by implementation of HierarchicalClusteringIndexParams (Bug #2397)

Note: Returning the if statement mentioned in the description is enough to fix support for LshIndexParams. This is just one example of special case handling (hacks?) for LshIndexParams, there were others too. It would probably be best to revert the commit and cleanly implement support again.

Sorry,

"Returning the if statement mentioned in the description is enough to fix support for LshIndexParams"
should read:
"Returning the if statement mentioned in the description is not enough to fix support for LshIndexParams"

Hi,

Do you have an example of code that got broken by the commit? The if should be handled by the case branch:

case FLANN_DIST_HAMMING:
        buildIndex< HammingDistance >(index, data, params);
        break;

Hi,
Marius did we need also a knnSearch wrap method of findNeighbors as implemented in lsh_index.h?

Should be fixed in 5cf6c5f0b273cf73be623c1df4ea2a01c3c4be8a, Benjamin can you confirm?

Stefano, the knnSearch method in lsh_index.h overwrites the one inherited from the NNIndex to ensure a ResultSet storing only unique neighbors is used.

Ok so do we need to call directly findNeighbors? Because documentation and miniflann expose only knnSearch and radiusSearch actually with flann::Index_<T>::knnSearch and flann::Index_<T>::radiusSearch.

No, you should not call findNeighbors directly (it should actually be a protected member), it's being called by knnSearch and radiusSearch.

Marius Muja wrote:

Should be fixed in 5cf6c5f0b273cf73be623c1df4ea2a01c3c4be8a, Benjamin can you confirm?

Thanks, that seems to have done the trick.

You mention in your commit message that this change is to enable backwards compatibility. What is the correct (i.e. modern) way to use LSH indexes now?

Presently I'm doing something like:

FlannBasedMatcher flannBasedMatcher(new flann::LshIndexParams(kLshTableNumber, kLshKeySize, kLshMultiProbeLevel);