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 to make compatible with MarginRankingCriterion #108
Conversation
Currently ModuleFromCriterion assumes that gradPrediction line 26 is a tensor. This assumption break for criterions like MarginRankingCriterion where it is a table instead. Adding code to make it compatible for criterions where prediction (as well as gradPrediction) can be a table.
Not sure why the build fails on LUA52 .. :( |
It still works, on others. Anyone? |
self.gradInput[1]:resizeAs(gradPrediction):copy(gradPrediction):mul(gradOutput[1]) | ||
if type(gradPrediction) == 'table' then | ||
if type(self.gradInput[1]) ~= 'table' then | ||
self.gradInput[1] = gradPrediction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in this case, aren't you forgetting to multiply by gradOutput[1]?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@soumith - Thanks for pointing out. Fixed it now. Hopefully this time around, it is correct.
The Lua 5.2 fails weren't your fault, I fixed them in trunk. Made an in-line comment for this PR. |
if type(self.gradInput[1]) ~= 'table' then | ||
self.gradInput[1] = {} -- initializing to table first time if it is tensor (which it is: line 10) | ||
for i=1, #gradPrediction do | ||
self.gradInput[1][i] = gradPrediction[i]:clone() -- and putting tensors of right size inside. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the clone is wasteful here. you just need:
self.gradInput[1][i] = gradPrediction[i].new()
below, you resize the tensor and copy the contents over.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@soumith - I agree. Changed it now as per your suggestion.
Thanks Abhi! |
Before the fix ModuleFromCriterion assumes that gradPrediction (line 26) is a tensor. This assumption breaks for criterions like MarginRankingCriterion where it is a table instead. Adding code to make it compatible for criterions where prediction (as well as gradPrediction) can be a table.