ArgMaxLayer with top k predictions

[kloudkl]

To meet the needs for multiple top predictions in #499 and #598, the argmax is extended to output the top k results.

Unlike the implementation of the top k accuracy layer in #531, the argmax layer doesn't assume the input probabilities to be sorted. It picks the top k results with a priority queue.

Candidate reviewers:
@sguada who authored #421 Argmax layer
@robwhess who authored #531 Top-k accuracy
@shuokay who asked #598 How to make top-k prediction

[sguada]

I would use top_k_(5)

[kloudkl]

@sguada, thanks for your reviewing efforts! The new functionality is tested more thoroughly and ready to be merged!

[robwhess]

@kloudkl, is there a cleaner way to index into top_data here and down on line 73? E.g. using Blob::offset() seems like it'd be a lot cleaner/easier to read.

[robwhess]

@kloudkl, this may just be a biased preference for my own code, but for the sake of consistent implementation of the same functionality, I'd rather see the truncated insertion sort from the AccuracyLayer (accuracy_layer.cpp lines 46-55) used to find the top k probabilities and their corresponding indices here as well instead of the priority queue. It seems strange to me to implement the same thing two different ways in the same project, and I have a slight aesthetic preference for the code I wrote :)

[sguada]

I agree with @robwhess don't reinvent new code, try to reuse as much as possible.

Contrary to your claim, #531 don't assume that the inputs are ordered.

[kloudkl]

The sorting algorithm is only a small part of the whole system, so it may not be worth much optimization. In terms of readability and conciseness, the std::sort is no doubt the winner. If you agree, I'd like to unify all the sorting codes to directly reuse it rather than "reinvent new code".

Insertion sort

Best case: O(n + d), where d is the number of inversions
Average case: O(n * n)
Worst case: O(n * n)
Memory: O(1)

Heapsort

Best case: O(n * log n) 
Average case: O(n * log n)
Worst case: O(n * log n)
Memory: O(1)

std::sort

O(N·log(N)), where N = std::distance(first, last) comparisons on average. (until C++11)
O(N·log(N)), where N = std::distance(first, last) comparisons. (since C++11)

[robwhess]

@kloudkl we don't need to sort all of the probabilities, just pick the top 5 (or k). That's what you're doing with the priority queue, and that's what I'm doing with the truncated insertion sort in AccuracyLayer. Both of our approaches are O(n), though I doubt algorithmic complexity is a huge concern for this layer. I'm just suggesting we use the same approach both places, and I prefer the truncated insertion sort because it's less bulky (no extra comparator class), and I think easier to read and understand.

[shuokay]

@kloudkl ,I think std::partial_sort can help
@robwhess

[kloudkl]

@shuokay, very cool!

[robwhess]

@kloudkl is there a reason we need a stable sort here? Can't we drop the second term of the OR here? It seems both unlikely that classes will have exactly the same probability and, if they do, unimportant that we keep them in the (arbitrary) order of the assigned indices. The second term here just seems to me to just add extra clutter to the code.

[robwhess]

Oh, also, I just realized this was in common_layers.hpp. That doesn't seem like the right place for this. Can we just put it in argmax_layer.cpp as _int_Dtype_pair_greater()? I think that'd make more sense.

[robwhess]

Yes, I like this solution quite a bit better than both the original solutions. I didn't know about partial_sort(). Would it be appropriate to make a corresponding change to the AccuracyLayer in this PR?

Other than that, the only concern I have with this PR is the line comment I made above.

[robwhess]

Oh, one more thing. There's a lint error introduced:

src/caffe/layers/argmax_layer.cpp:39:  Add #include <utility> for pair<>  [build/include_what_you_use] [4]

@kloudkl, can you add #include <utility> to argmax_layer.cpp?

[kloudkl]

@robwhess, everything you requested is in place.

[robwhess]

@kloudkl, that all looks good. One more very minor request: can you make the int_Dtype_pair_greater() functions static and begin their names with an underscore so it's clear that they only have file scope?

[kloudkl]

In Caffe, there is not a single function or method with a leading or training underscore. It's better to follow the convention.

[robwhess]

OK. That's fine, though to be fair, there also aren't any static functions in the src/caffe/ portion of the code. It's just a personal preference, though. I'll run tests tomorrow.

[robwhess]

Cool, tests are all passing for me.

@sguada, @shelhamer, @jeffdonahue, @longjon, @sergeyk, I think this PR can be merged.

[robwhess]

Oh, wait, nevermind, don't merge yet. @kloudkl, the lint error I mentioned above now also applies to accuracy_layer.cpp. Can you add #include <utility> there before this is merged?

[kloudkl]

@robwhess, it is added. Thanks for your help!

[robwhess]

Cool. I think we're ready to merge here.

[bhack]

Could be extended to support a vector of bottom blob labels instead of single blob?

[robwhess]

@bhack I think that's out of the scope of this PR. This should be merged, as is.

[shelhamer]

Ok looks good to me, but I'm traveling and only took a quick glance so @longjon please review and merge.

Thanks for your work everybody!

[bhack]

@kloudkl @robwhess Handling a vector of bottom (i.e. multiple softmax) could be useful for this #596 #680

[longjon]

Please use curly braces for loop bodies. (Although Google C++ style guide doesn't require it for single line statements, as far as I know we always use explicit curly braces in Caffe.)

[longjon]

Looks good except as noted. I like using std::partial_sort; building vectors is probably far from an optimally efficient implementation, but there's no reason to worry about that for new functionality that's not a bottleneck.

[kloudkl]

All done. Any more concerns?

[longjon]

Merged. I took the liberty of removing some now-unused includes. Thanks @kloudkl and @robwhess for getting this done; I think we ended up with a nice, tight implementation.