Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug in evaluation for tie in scores. #36

Merged
merged 2 commits into from
Jul 26, 2018
Merged

Fix bug in evaluation for tie in scores. #36

merged 2 commits into from
Jul 26, 2018

Conversation

kdexd
Copy link
Member

@kdexd kdexd commented Jul 25, 2018

If the score of ground truth would tie with another option, it always got assigned a higher rank. This caused evaluation metrics to be reported higher than their true values. This PR fixes that.

utils.lua Outdated
ranks = ranks:view(-1, numOpts)
local gtRanks = torch.LongTensor(gtPos:size(1))
for i = 1, gtPos:size(1) do
gtBinary = torch.LongTensor(numOpts):zero()
Copy link
Member

Choose a reason for hiding this comment

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

gtBinary doesn't need to be a global variable, right?

utils.lua Outdated
for i = 1, gtPos:size(1) do
gtBinary = torch.LongTensor(numOpts):zero()
gtBinary[{gtPos[{i}]}] = 1
sorted, rankedIdx = ranks[{i}]:sort()
Copy link
Member

Choose a reason for hiding this comment

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

Wait, can't we re-use rankedIdx from L108 instead of recomputing it again for every round?

utils.lua Outdated
gtBinary[{gtPos[{i}]}] = 1
sorted, rankedIdx = ranks[{i}]:sort()
sortedGt = gtBinary:index(1, rankedIdx:long())
gtRank = sortedGt:nonzero()
Copy link
Member

Choose a reason for hiding this comment

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

sortedGt and gtRank don't need to be global either.

@abhshkdz
Copy link
Member

@kdexd could you review 3a769d2 please.

utils.lua Outdated
sortedGt = gtBinary:index(1, rankedIdx:long())
gtRank = sortedGt:nonzero()
gtRanks[i] = gtRank
gtRanks[i] = ranks[{i, gtPos[i]}]
Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I see how this works... I tried advanced indexing to avoid this loop (using multiple LongTensors) and when it raised errors about lack of support, it suffered over-simplification (straightaway as the EvalAI script does it).

I tested this to cross verify and it works fine too. We are good to go with it!

@abhshkdz abhshkdz merged commit 5b39e47 into master Jul 26, 2018
@kdexd kdexd deleted the scoretie-fix branch July 26, 2018 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants