Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Fix example/reinforcement-learning/dqn #9128

Merged
merged 3 commits into from
Dec 19, 2017
Merged

Fix example/reinforcement-learning/dqn #9128

merged 3 commits into from
Dec 19, 2017

Conversation

mbaijal
Copy link
Contributor

@mbaijal mbaijal commented Dec 19, 2017

Description

  • This example was not compatible with python3. I made necessary changes.
  • I also updated the README with the Pre-reqs I needed to make the example work on the CONDA AMI (mxnet_p36/mxnet_p27 envs)

Checklist

Essentials

  • Passed code style checking (make lint)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • Some changes to atari_game.py to make it compatible with both python2 and python3.
  • Some changes to base.py to make it compatible with python2 and python3.
  • Updated README

Comments

  • I have tested on the Deep Learning AMI with Conda (Ubuntu) (ami-405ade3a)
  • I fixed it such that it works with both python 3.x and python 2.x
  • I have not verified the accuracy or validity of results.
  • I have not tested on Mac OS

@mbaijal
Copy link
Contributor Author

mbaijal commented Dec 19, 2017

@sxjscience Can you please help me review the changes here.
Thanks.

mbaijal
mbaijal commented Dec 19, 2017
View reviewed changes
example/reinforcement-learning/dqn/base.py
@@ -135,7 +135,7 @@ def switch_bucket(self, bucket_kwargs=None, data_shapes=None):
self.initializer(k, v)
else:
assert set(arg_name_shape.items()) == \
set(data_shapes.items() + [(k, v.shape) for k, v in self.params.items()])
set(list(data_shapes.items()) + list([(k, v.shape) for k, v in self.params.items()]))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In python3.x, this can also be fixed by changing to union of dict_items as -
set(data_shapes.items() | dict([(k, v.shape) for k, v in self.params.items()]).items())

However, this fails in python2.7 as this results in a union of lists.

Copy link
Member

Choose a reason for hiding this comment

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

This fix looks good.

@sxjscience
Copy link
Member

@mbaijal Can it be run using the AMI after these changes?

@piiswrong piiswrong merged commit d1d3e97 into apache:master Dec 19, 2017
rahul003 pushed a commit to rahul003/mxnet that referenced this pull request Jun 4, 2018
@mbaijal @piiswrong
Fix example/reinforcement-learning/dqn (apache#9128)
199883b 
* Some changes to make dqn compatible with python 3

* Update README

* union of dict_items does not work in python2.7. Change to list
zheng-da pushed a commit to zheng-da/incubator-mxnet that referenced this pull request Jun 28, 2018
@mbaijal @zheng-da
Fix example/reinforcement-learning/dqn (apache#9128)
e0fcdf8 
* Some changes to make dqn compatible with python 3

* Update README

* union of dict_items does not work in python2.7. Change to list
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sxjscience sxjscience sxjscience left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mbaijal @sxjscience @piiswrong