Skip to content

Improve StateActionQFunctions#172

Merged
toslunar merged 11 commits intochainer:masterfrom
muupan:improve-saqf
Nov 16, 2017
Merged

Improve StateActionQFunctions#172
toslunar merged 11 commits intochainer:masterfrom
muupan:improve-saqf

Conversation

@muupan
Copy link
Copy Markdown
Member

@muupan muupan commented Nov 15, 2017

Please merge this after merging #171

  • Add nonlinearity and last_wscale aruguments to StateActionQFunctions
  • Improve docstrings
  • Add tests for StateActionQFunctions. They are actually not checking if each configuration is applied or not, but it should be better than having no test.

@muupan muupan changed the title Improve StateActionQFunctions [WIP] Improve StateActionQFunctions Nov 15, 2017
@muupan muupan changed the title [WIP] Improve StateActionQFunctions Improve StateActionQFunctions Nov 15, 2017
Copy link
Copy Markdown
Member

@toslunar toslunar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I left a few minor comments.

Nonlinearities with learnable parameters such as PReLU are not
supported.
last_wscale (float): Scale of weight initialization of the last layer.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This empty line seems unnecessary.

self.nonlinearity = nonlinearity

super().__init__()
with self.init_scope():
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment that nonlinearity does not need to be passed to MLPBN because hidden_sizes is empty?

*testing.product({
'n_dim_obs': [1, 5],
'n_dim_action': [1, 3],
'n_hidden_layers': [0, 1, 2],
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'n_hidden_layers': [1, 2],

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

n_hidden_layers=0 works except for LateAction architectures. I'll remove 0 for them.

@muupan
Copy link
Copy Markdown
Member Author

muupan commented Nov 16, 2017

Thank you for your review! I fixed the points you mentioned.

@toslunar toslunar merged commit 5aa8d6e into chainer:master Nov 16, 2017
@muupan muupan added this to the v0.3 milestone Nov 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants