Separate feature extractor networks for DQN networks

[Miffyli]

Closes #131

Description

DQN main and target network accidentally shared feature extractor network. This fix creates new feature extractor each time new QNetwork is made.

Motivation and Context

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation (update in the documentation)

Checklist:

• I've read the CONTRIBUTION guide (required)
• I have updated the changelog accordingly (required).
• My change requires a change to the documentation.
• I have updated the tests accordingly (required for a bug fix or a new feature).
• I have updated the documentation accordingly.
• I have reformatted the code using make format (required)
• I have checked the codestyle using make check-codestyle and make lint (required)
• I have ensured make pytest and make type both pass. (required)

[araffin]

Nice catch, but it was working before?

I think we may have the issue with SAC/TD3 too then.

[Miffyli]

Nice catch, but it was working before?

I do not see how it could have worked before, unless somewhere along the line the models got cloned or the references to data they use were separated (not sure how that could happen, either...).

As for SAC/TD3, I heard they share the feature extractor part but this is not updated as part of target-update, right? In DQN you update the whole network, and thus you have full copies of both.

Edit: No wait, we might have the same issue, as it seems like whole networks are being updated. Those will require more careful testing to see what is right for SAC/TD3.

[araffin]

LGTM, nice catch =)

Do you have an easy way of testing that? Otherwise, I will merge as soon CI passes.

For SAC/TD3, we will see later, as I would like to share the feature extractor between actor and critic (otherwise it is super slow, and even when sharing, it is slow)

[m-rph]

If the extractor isn’t flatten, then using the “is” operator with the .parameters() function should catch it.

…

On 30 Jul 2020, at 21:36, Antonin RAFFIN ***@***.***> wrote:  @araffin approved this pull request. LGTM, nice catch =) Do you have an easy way of testing that? Otherwise, I will merge as soon CI passes. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[araffin]

If the extractor isn’t flatten, then using the “is” operator with the .parameters() function should catch it.

Ok, I think I will delay the test to SAC/TD3 fix, as this is a hotfix for DQN.