Skip to content
This repository has been archived by the owner on Sep 20, 2022. It is now read-only.

[HIVEMALL-124][BUGFIX] Fixed bugs in BinaryResponseMeasure (nDCG, MRR, AP) #115

Closed
wants to merge 7 commits into from

Conversation

myui
Copy link
Member

@myui myui commented Sep 15, 2017

What changes were proposed in this pull request?

Fixed bug in the BinaryResponse measure such as nDCG, MRR, (M)AP.

What type of PR is it?

Bug Fix

What is the Jira issue?

https://issues.apache.org/jira/browse/HIVEMALL-124

How was this patch tested?

unit tests, manual tests

Checklist

  • Did you apply source code formatter, i.e., mvn formatter:format, for your commit?
  • Did you run manual test on Hive/Spark CLI on the modified functions?

@coveralls
Copy link

coveralls commented Sep 15, 2017

Coverage Status

Coverage decreased (-0.05%) to 40.927% when pulling bef36ed on myui:HIVEMALL-124 into 06f2f82 on apache:master.

@myui
Copy link
Member Author

myui commented Sep 15, 2017

@takuti could you review this PR?

@coveralls
Copy link

coveralls commented Sep 15, 2017

Coverage Status

Coverage decreased (-0.05%) to 40.926% when pulling eb0cb36 on myui:HIVEMALL-124 into 06f2f82 on apache:master.

}
return 0.d;
}
final int k = Math.min(rankedList.size(), recommendSize);
Copy link
Member

Choose a reason for hiding this comment

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

Computing Math.min(rankedList.size(), recommendSize) twice at here and inside of TruePositives looks redundant and a little bit confusing. For the sake of simplicity, how about directly writing what TruePositives does both in Precision and Recall, and remove TruePositives? Even though the same for-loop appears twice, I personally feel it's much more easier to understand.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 759b088

Recall is as it is.

if (nTruePositive == 0) {
return 0.d;
}
return sumPrecision / nTruePositive;
Copy link
Member

Choose a reason for hiding this comment

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

👍

@myui
Copy link
Member Author

myui commented Sep 15, 2017

Ideally, it should be ndcg(array(1,3), array(1)) < ndcg(array(1), array(1)) but correct code return 1.0 for both cases.

@coveralls
Copy link

coveralls commented Sep 15, 2017

Coverage Status

Coverage decreased (-0.04%) to 40.929% when pulling cd5cb4a on myui:HIVEMALL-124 into 06f2f82 on apache:master.

@myui myui changed the title [WIP][HIVEMALL-124][BUGFIX] Fixed bugs in BinaryResponseMeasure (nDCG, MRR, AP) [HIVEMALL-124][BUGFIX] Fixed bugs in BinaryResponseMeasure (nDCG, MRR, AP) Sep 15, 2017
@asfgit asfgit closed this in c2b9578 Sep 15, 2017
@coveralls
Copy link

coveralls commented Sep 15, 2017

Coverage Status

Coverage decreased (-0.04%) to 40.929% when pulling d70c072 on myui:HIVEMALL-124 into 06f2f82 on apache:master.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants