hal3 / macarico

learning to search in pytorch
MIT License
111 stars 12 forks source link

Completed issue 31: Support Reward Per time-step #35

Open xkianteb opened 6 years ago

hal3 commented 6 years ago

shouldn't this be a tail sum?

amrsharaf commented 6 years ago

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.

hal3 commented 6 years ago

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

amrsharaf commented 6 years ago

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?

hal3 commented 6 years ago

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

khanhptnk commented 6 years ago

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.