Element-Research / rnn

Recurrent Neural Network library for Torch7's nn
BSD 3-Clause "New" or "Revised" License
939 stars 313 forks source link

Add sizeAverage to SequencerCriterion #342

Closed hfxunlp closed 7 years ago

hfxunlp commented 7 years ago

The sizeAverage variable of the criterion decorated by the SequencerCriterion controls whether output the sum or the average of the losses of the whole sequence.

JoostvDoorn commented 7 years ago

Maybe you can add an argument to the constructor, and initialize a default value for sizeAverage? This would make it similar to ClassNLLCriterion. Also document the default behaviour.

hfxunlp commented 7 years ago

@JoostvDoorn I dare to change the API, and I think if the criterion was decorated does not average the losses, then the squence should be processed in the same way, vice versa, so I change it in this way. But anyhow I'm a new man, I hope you could help me to change the program in a proper way.

JoostvDoorn commented 7 years ago

No problem. You can do this:

function SequencerCriterion:__init(criterion, sizeAverage)
   parent.__init(self)
   self.criterion = criterion
   if torch.isTypeOf(criterion, 'nn.ModuleCriterion') then
      error("SequencerCriterion shouldn't decorate a ModuleCriterion. "..
         "Instead, try the other way around : "..
         "ModuleCriterion decorates a SequencerCriterion. "..
         "Its modules can also be similarly decorated with a Sequencer.")
   end
   if sizeAverage ~= nil then
      self.sizeAverage = sizeAverage
   else
      self.sizeAverage = false
   end
   self.clones = {}
   self.gradInput = {}
end
hfxunlp commented 7 years ago

@JoostvDoorn Thank you :-)

JoostvDoorn commented 7 years ago

I think you unintentionally closed this pull request. 😄

hfxunlp commented 7 years ago

@JoostvDoorn Sorry, I am also a new man to github, I change the code in my repository with your advice and thinks that is ok now, so I closed it.

nicholas-leonard commented 7 years ago

Hi @anoidgit thanks for the excellent PR. I extended it to have sizeAverage=true affect gradInput as well. This is how it is done for the rest of torch (e.g. https://github.com/torch/nn/blob/master/MSECriterion.lua#L29).

@JoostvDoorn Thanks for lending a hand :)

hfxunlp commented 7 years ago

@nicholas-leonard It is my pleasure, I love this library very much. @JoostvDoorn My true gratitude for your generous help is beyond the word's description as I am a new guy here.