Added support for clipping to DQNPolicy.

[michalgregor]

Added support for clipping to DQNPolicy.
* When clip_delta=True is passed, smooth_l1_loss is used
instead of the MSE loss.

• I have marked all applicable categories:
  • exception-raising fix
  • algorithm implementation fix
  • documentation modification
  • new feature
• I have reformatted the code using make format (required)
• I have checked the code using make commit-checks (required)
• If applicable, I have mentioned the relevant/related issue(s)
• If applicable, I have listed every items in this Pull Request below

[Trinkle23897]

Is it possible to add this feature for all DQN family?

[michalgregor]

Is it possible to add this feature for all DQN family?

Yes, I can go through them and make the changes. Do you have any idea which ones I will need to look into? E.g. there is a component in SAC that is trained DQN-style – should I also look there, for example?

Also, I am currently using smooth L1 (I saw it being used in another implementation), but PyTorch also has Huber loss...

Finally, the implementation is a bit redundant – the loss expects the output and the target – but since we also need td_error for the prio buffer, the way I implemented this, we do subtraction twice. But I am not sure how best to avoid that...

[Trinkle23897]

Do you have any idea which ones I will need to look into? E.g. there is a component in SAC that is trained DQN-style – should I also look there, for example?

After a second check of the current codebase, I think it's fine for this PR to only modify DQN loss. It's because other methods, e.g., IQN / FQF / QRDQN, they use a distribution to calculate loss, and it's non-trivial to directly apply this smooth_l1_loss. (BTW QRDQN has already use smooth_l1_loss to some extend)

Also, I am currently using smooth L1 (I saw it being used in another implementation), but PyTorch also has Huber loss...

Could you please elaborate more on it?

Finally, the implementation is a bit redundant – the loss expects the output and the target – but since we also need td_error for the prio buffer, the way I implemented this, we do subtraction twice. But I am not sure how best to avoid that...

That's fine.

[michalgregor]

Also, I am currently using smooth L1 (I saw it being used in another implementation), but PyTorch also has Huber loss...

Could you please elaborate more on it?

I have seen DQN code using the smooth L1 loss, but as I recalled, the DQN paper uses the Huber loss – so I figured someone here might know why one would choose one over the other. But looking into it properly just now, I see that with the default parameters (beta=1 for the smooth L1 and delta=1 for the Huber loss), they are actually exactly the same, so there is no difference. Sorry for the confusion.

[codecov-commenter]

Codecov Report

❗ No coverage uploaded for pull request base (master@c87b9f4). Click here to learn what that means.
The diff coverage is 50.00%.

@@            Coverage Diff            @@
##             master     #642   +/-   ##
=========================================
  Coverage          ?   93.56%           
=========================================
  Files             ?       71           
  Lines             ?     4755           
  Branches          ?        0           
=========================================
  Hits              ?     4449           
  Misses            ?      306           
  Partials          ?        0           

Flag	Coverage Δ
unittests	93.56% <50.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
tianshou/policy/modelfree/dqn.py	94.38% <50.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more