# ~~Possible PPO surrogate policy loss sign error~~

**URL:** <https://discuss.ray.io/t/possible-ppo-surrogate-policy-loss-sign-error/7601>\
**Category:** RLlib\
**Created:** [September 19, 2022, 1:00pm UTC](https://discuss.ray.io/t/possible-ppo-surrogate-policy-loss-sign-error/7601 "2022-09-19T13:00:18Z")\
**Posts on this page:** 3\
**Page:** 1

<div class="post-metadata">

**Author:** ![mannyv](https://sea2.discourse-cdn.com/flex020/user_avatar/discuss.ray.io/mannyv/32/606_2.png) [@mannyv](https://discuss.ray.io/u/mannyv)\
**Post date:** [September 19, 2022, 1:00pm UTC](https://discuss.ray.io/t/possible-ppo-surrogate-policy-loss-sign-error/7601/1 "2022-09-19T13:00:18Z")

</div>

Hello,

I think there is an error in the PPO clipped surrogate loss and would appreciate if someone else could take a look and let me know if I might be missing something. This affects both the tf and torch versions.

> <https://github.com/ray-project/ray/blob/7280ef4d466a7935de8cfaa36785439733019d09/rllib/algorithms/ppo/ppo_torch_policy.py#L132-L140>

> <https://github.com/ray-project/ray/blob/7280ef4d466a7935de8cfaa36785439733019d09/rllib/algorithms/ppo/ppo_torch_policy.py#L156-L160>

If you look at the code snippet above from master. You will see that line 139 computes the mean of the negative surrogate loss. But, on line 175 when combining the surrogate, vf, and entropy losses to get the total\_loss it is negated again, which I think is incorrect.

I checked the cleanrl implementation and observed that they compute it as I expected with only one negative.

> <https://github.com/vwxyzjn/cleanrl/blob/e466f6efb251462de2b80f064133f32cb5d83e22/cleanrl/ppo.py#L275-L278>

> <https://github.com/vwxyzjn/cleanrl/blob/e466f6efb251462de2b80f064133f32cb5d83e22/cleanrl/ppo.py#L296>

as does stable baselines:

> <https://github.com/DLR-RM/stable-baselines3/blob/d0b129ecc3b221fd5aeacd0d44d9ee7ff7dd00ee/stable_baselines3/ppo/ppo.py#L222-L224>

> <https://github.com/DLR-RM/stable-baselines3/blob/d0b129ecc3b221fd5aeacd0d44d9ee7ff7dd00ee/stable_baselines3/ppo/ppo.py#L253>

and although it is harder to track so does seedrl (I think):

> <https://github.com/google-research/seed_rl/blob/66e8890261f09d0355e8bf5f1c5e41968ca9f02b/agents/policy_gradient/modules/policy_losses.py#L147-L171>

---

<div class="post-metadata">

**Author:** ![mannyv](https://sea2.discourse-cdn.com/flex020/user_avatar/discuss.ray.io/mannyv/32/606_2.png) [@mannyv](https://discuss.ray.io/u/mannyv)\
**Post date:** [September 19, 2022, 8:28pm UTC](https://discuss.ray.io/t/possible-ppo-surrogate-policy-loss-sign-error/7601/2 "2022-09-19T20:28:34Z")

</div>

@Lars_Simon_Zehnder pointed out that `mean_policy_loss` is used for metrics and `surrogate_loss` is used for the loss.

I think then it is OK.

What threw me off is that most libraries I look at take the mean of each component separately and then combine them but rllib is combining them then taking the mean. So I was expecting the `mean_policy_loss` to be the one in total\_loss and I did not read carefully enough.

Why is the mean re-computed in total\_loss? Perhaps required for multi-gpu cases?

---

<div class="post-metadata">

**Author:** ![gjoliver](https://sea2.discourse-cdn.com/flex020/user_avatar/discuss.ray.io/gjoliver/32/1490_2.png) [@gjoliver](https://discuss.ray.io/u/gjoliver)\
**Post date:** [October 4, 2022, 6:19pm UTC](https://discuss.ray.io/t/possible-ppo-surrogate-policy-loss-sign-error/7601/3 "2022-10-04T18:19:04Z")

</div>

feel like this is just bad code.  
let me make a clean up PR and see how things go.