<p>In <a href="https://github.com/mlpack/mlpack/pull/732#discussion_r71900589">src/mlpack/methods/cf/cf.hpp</a>:</p>
<pre style='color:#555'>> + {
> + //! Value of this recommendation.
> + double value;
> + //! Item of this recommendation.
> + size_t item;
> + //! Trivial constructor.
> + Candidate(double value, size_t item) :
> + value(value),
> + item(item)
> + {};
> + //! Compare the value of two candidates.
> + friend bool operator>(const Candidate& l, const Candidate& r)
> + {
> + return l.value > r.value;
> + };
> + };
</pre>
<p><a href="https://github.com/rcurtin" class="user-mention">@rcurtin</a> , yes I have considered std::pair<> but I thought it would be clearer if we have specific structure for this (maybe value/item is clearer than first/second...). Of course, I can use std::pair<> if you think it would be better :)<br>
However, I can see a problem with the relational operators of std::pair.<br>
They compare the first element and, if these elements are equal, they compare the second element. So, I think we should set "first" to represent the value, and "second" to represent the index.<br>
When we decide if inserting a new element to the list of candidates, we make a comparison with the top element: "if (c > pqueue.top())"<br>
If we use a pair, it will insert c in the case where:</p>
<p>c.first == pqueue.top().first && c.second > pqueue.top().second</p>
<p>This is an unnecesary overhead, because we don't care on the order of the indexes, so we are inserting a new element with the same value but a greater index...</p>
<p>We could easily fix this by explicitly comparing the first element: "if (c.first > pqueue.top().first)"</p>
<p>However, I can see another possible disadvantage. Inside the implementation of the priority queue, if we use the std::pair relational operator, elements will be not only sorted by value but also by index. This can cause a bit more overhead when shifting down/up through the heap.</p>
<p>Maybe I became a bit obsessed with efficiency and this doesn't make a real difference...</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/mlpack/mlpack/pull/732/files/57dd61f1d823f209c8d0f67fa69f8a8ae0669d18#r71900589">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AJ4bFO9KPRA5LtT6v3zDKIzizMOkVYwpks5qYOYfgaJpZM4JScnZ">mute the thread</a>.<img alt="" height="1" src="https://github.com/notifications/beacon/AJ4bFJr15mgTU54PrJ7irfnovdvlWET4ks5qYOYfgaJpZM4JScnZ.gif" width="1" /></p>
<div itemscope itemtype="http://schema.org/EmailMessage">
<div itemprop="action" itemscope itemtype="http://schema.org/ViewAction">
<link itemprop="url" href="https://github.com/mlpack/mlpack/pull/732/files/57dd61f1d823f209c8d0f67fa69f8a8ae0669d18#r71900589"></link>
<meta itemprop="name" content="View Pull Request"></meta>
</div>
<meta itemprop="description" content="View this Pull Request on GitHub"></meta>
</div>