AI4Finance-Foundation / AI4Finance-Foundation/FinRL

Wrong reward at terminal date when training with SB3

Ouverte
#1,228 0 commentaires 0 réactions 0 personnes assignées Voir sur GitHub
Langage dominant
Jupyter Notebook
Étoiles
16.3k
Forks
3.5k
Métriques de merge des PR
Aucune PR mergée en 30 j

Description

**Describe the bug**
For StockTradingEnv when training in SB3 VecEnv (using get_sb_env), at the terminal date(let's say day X), it doesn't calculate reward since training data doesn't contain the next date closed price.
But when SB3 algorithm conducts collect_rollouts, it adds the previous reward (day X-1's reward) into rollout_buffer.

**To Reproduce**
Steps to reproduce the behavior:
1. pick a short period training data to quickly reach terminal date when debuging , eg. 5days in total.
2. break point after rollout_buffer.add() in on_policy_algorithm.py(SB3 module) line 232
3. stepping until after day 5 (add the 4th experience)
4. see the rollout_buffer variable (the reward[3] is the same as the previous one)

**Expected behavior**
after I examined SB3 code, it provides a solution for calculating the reward when next state is unobservable in episode. it use prediction value with discount as reward, while I'm not sure about its mathematic meaning.
```
# see GitHub issue #633
for idx, done in enumerate(dones):
if (
done
and infos[idx].get("terminal_observation") is not None
and infos[idx].get("TimeLimit.truncated", False)
):
terminal_obs = self.policy.obs_to_tensor(infos[idx]["terminal_observation"])[0]
with th.no_grad():
terminal_value = self.policy.predict_values(terminal_obs)[0] # type: ignore[arg-type]
rewards[idx] += self.gamma * terminal_value
```

But it requires the Env return "terminated = False, truncated = True." (while StockTradingEnv return "terminated = True, truncated = False:) # In SB3, truncated and terminated are mutually exclusive.

I think in stock trading scenario, the last day in training data should be a time limit truncation, but not a terminal, because it has future reward (value), unless it holds zero shares and the policy will not buy anymore at that state.

**Screenshots**
![image](https://github.com/AI4Finance-Foundation/FinRL/assets/81505112/585e033b-22bb-4c47-8b2f-247c92f3539d)

**Additional context**
I simply change the return in terminal case in the StockTradingEnv code, line 300.
`return self.state, 0.0, False, True, {}`. # reward = 0, could cause some issue

but I didn't consider the impact on ElegantRL, Ray and Portfolio Management scene.

Guide de contribution

Aucun guide de contribution indexé pour ce dépôt

Évaluation

Cette issue n'a pas encore été évaluée.

Recevez les nouvelles issues par e-mail

Un résumé court des issues GitHub adaptées aux débutants.