Diversity metric _ inter-list diversity #14
Conversation
Signed-off-by: Xin Wang <xin.wang@fmr.com>
Signed-off-by: Xin Wang <xin.wang@fmr.com>
There was a problem hiding this comment.
Went over the PR, and it looks great overall! Left a few comments on things that can change, primarily some documentation changes and reusing some code that was already a part of Jurity. Also left a few questions for my understanding.
The implementation of the metric itself looks good. Happy to approve once the two changes I mentioned above are done. Thanks for the fast return!
|
Thanks for the PR @takojunior ! I think @dorukkilitcioglu had few more important suggestions about the API signatures. |
Signed-off-by: Xin Wang <xin.wang@fmr.com>
There was a problem hiding this comment.
@takojunior thank you for jumping on this quickly!
This was sitting in the backlog for a while and having the functionality available in Jury will help a lot to make the RecWiser notebooks leaner and facilitate our collaboration.
Some minor comments and 1 issue that worth considering before a Merge about the sample_size parameter and it's default behavior.
Great addition to Jurity!!
Signed-off-by: Xin Wang <xin.wang@fmr.com>
Signed-off-by: Xin Wang <xin.wang@fmr.com>
Signed-off-by: Xin Wang <xin.wang@fmr.com>
|
Thanks @skadio @filip-michalsky @dorukkilitcioglu for the constructive comments. A few updates have been applied to the PR. Please let me know if there are further comments. |
There was a problem hiding this comment.
The changes look good! Thanks for the contribution @takojunior!
|
@takojunior all good, thanks a lot for the quick turnaround! I left few very minor comments, otherwise good to merge. |
Signed-off-by: Xin Wang <xin.wang@fmr.com>
Signed-off-by: Xin Wang <xin.wang@fmr.com>
This pull request is for adding inter-list diversity to the recommender metrics. Since the implementation of inter-list diversity doesn't support the
batch_accumulateusage, minor changes are applied toCombinedMetrics. New tests are added accordingly and all tests get passed. The other diversity metric planned to be added is intra-list diversity, while it could be better to get this one cleared first before having further additions to diversity metrics.