macarico icon indicating copy to clipboard operation
macarico copied to clipboard

Completed issue 31: Support Reward Per time-step

Open xkianteb opened this issue 7 years ago • 6 comments

xkianteb avatar Nov 12 '17 23:11 xkianteb

shouldn't this be a tail sum?

hal3 avatar Nov 15 '17 21:11 hal3

Yes, loss[i] is the tail sum of losses starting at timestep i. We can do this at reinforce.py (take loss per time step and convert it to tail sums) or assume the environment already gives us the array of tail sums, I think handling this at the environment side is cleaner.

amrsharaf avatar Nov 15 '17 21:11 amrsharaf

i'm not sure i agree. why should the env have to know how the RL algo works? and also not all RL algs will want tail sums

hal3 avatar Nov 15 '17 21:11 hal3

right, then I think the other way to do it is to accept loss per time step vector and do the tail summing in reinforce? (gamma should be a parameter to reinforce then) Is there a better way?

amrsharaf avatar Nov 15 '17 21:11 amrsharaf

ugh gamma. i think this should be an argument of the RL algorithm. there's good reason (eg Nan Jiang's work) to think you migth want to learn with a different gamma than the environment's gamma for regularization reasons anyway

hal3 avatar Nov 15 '17 21:11 hal3

Always returning tail sums may not be optimal. Sometimes you want to do something with reward like rescaling, injecting noise, exponentiate, etc., It think it is better to return per-step rewards and let the algorithm decide what to do with them. Correct me if I misunderstood this conversation.

khanhptnk avatar Nov 15 '17 22:11 khanhptnk