Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

a bug-fix of QR-DQN network definition. #157

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ddlau
Copy link

@ddlau ddlau commented Nov 24, 2020

No description provided.

@google-cla
Copy link

google-cla bot commented Nov 24, 2020

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Nov 24, 2020
@ddlau
Copy link
Author

ddlau commented Nov 24, 2020

@googlebot I signed it!

@google-cla google-cla bot added cla: yes CLA has been signed. and removed cla: no labels Nov 24, 2020
@ddlau
Copy link
Author

ddlau commented Nov 24, 2020

This is a Categorical-DQN legacy, which was just "transposed" in QR-DQN.
In contrast to Categorical-DQN, QR-DQN set fixed probabilites and learnable locations (a.k.a diracs).

@psc-g
Copy link
Collaborator

psc-g commented Nov 24, 2020

Thanks for pointing this out! This is indeed semantically incorrect and is mostly a consequence of QR-DQN inheriting from Rainbow (which does expect probabilities).
The probabilities field you rightly point out as not being probabilities is in fact not used at all in QR-DQN.

@ddlau
Copy link
Author

ddlau commented Nov 25, 2020

rightly

Yeah, I noticed it too, after "uptraced" to its caller, and found the strange thing is not used.
To be a little strict, isn't this sort of impreciseness and a misleading to new comings, more or less, I guess?
After all, it is not proper, so I suggest the modification, at least some comment there.

@psc-g
Copy link
Collaborator

psc-g commented Nov 25, 2020 via email

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
cla: yes CLA has been signed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants