Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Revision of CnnLstmPolicy with not None net_arch #1116

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

Conversation

HighExecutor
Copy link

@HighExecutor HighExecutor commented May 22, 2021

#1117 Description
When CnnLstmPolicy uses not None net_arch, cnn_extractor is used to perform features extraction inside LstmPolicy class. Here NotImplementedError is solved with usage of cnn_extractor. Test file is added.

Motivation and Context

  • I have raised an issue to propose this change (required for new features and bug fixes)

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 ensured pytest and pytype both pass (by running make pytest and make type).

…_acrh is used. Here NotImplementedError is solved with usage of cnn_extractor. Test file is added.
@araffin araffin added the PR template not filled Please fill the pull request template label May 23, 2021
@Miffyli
Copy link
Collaborator

Miffyli commented May 23, 2021

Thanks for the PR! Please complete all the required steps in the PR template.

@HighExecutor
Copy link
Author

I added the issue and edited PR.

@Miffyli Miffyli removed the PR template not filled Please fill the pull request template label May 26, 2021
@Miffyli
Copy link
Collaborator

Miffyli commented May 26, 2021

Hmm while I agree such functionality would be nice, it would differ from FeedForwardPolicy's behaviour where net_arch is ignored. I am not sure about changing functionality this late into the lifetime of the the package, or at very least both Lstm and FeedForward policies should have the same behaviour (in fact, FeedForward policy should probably throw an exception too on net_arch is not None).

@araffin comments? I favor not changing stuff, given maintenance mode of SB2.

@araffin
Copy link
Collaborator

araffin commented May 27, 2021

@araffin comments? I favor not changing stuff, given maintenance mode of SB2.

I would agree with this. We mostly accept only bug fixes and non-breaking changes now.
And if you need it, you can define a custom policy.

@carroll1100
Copy link

great. hope to see more of this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants