{"action":"created","issue":{"url":"https://api.github.com/repos/spragunr/deep_q_rl/issues/45","labels_url":"https://api.github.com/repos/spragunr/deep_q_rl/issues/45/labels{/name}","comments_url":"https://api.github.com/repos/spragunr/deep_q_rl/issues/45/comments","events_url":"https://api.github.com/repos/spragunr/deep_q_rl/issues/45/events","html_url":"https://github.com/spragunr/deep_q_rl/pull/45","id":107884757,"number":45,"title":"Refactor DataSet to use numpy arrays as circular buffers","user":{"login":"davidsj","id":10724404,"avatar_url":"https://avatars.githubusercontent.com/u/10724404?v=3","gravatar_id":"","url":"https://api.github.com/users/davidsj","html_url":"https://github.com/davidsj","followers_url":"https://api.github.com/users/davidsj/followers","following_url":"https://api.github.com/users/davidsj/following{/other_user}","gists_url":"https://api.github.com/users/davidsj/gists{/gist_id}","starred_url":"https://api.github.com/users/davidsj/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/davidsj/subscriptions","organizations_url":"https://api.github.com/users/davidsj/orgs","repos_url":"https://api.github.com/users/davidsj/repos","events_url":"https://api.github.com/users/davidsj/events{/privacy}","received_events_url":"https://api.github.com/users/davidsj/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":1,"created_at":"2015-09-23T09:53:09Z","updated_at":"2015-09-23T13:36:06Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/spragunr/deep_q_rl/pulls/45","html_url":"https://github.com/spragunr/deep_q_rl/pull/45","diff_url":"https://github.com/spragunr/deep_q_rl/pull/45.diff","patch_url":"https://github.com/spragunr/deep_q_rl/pull/45.patch"},"body":"Hi, thanks for all your work on a faithful reproduction of DQN!\r\n\r\nLet me know what you think of this change. I thought it might make sense to remove some complexity to make it easier to reason about what's going on (which may help to discover any discrepancies from the DeepMind DQN implementation).\r\n\r\nThis PR changes DataSet to use numpy arrays as a circular buffer, and adds some more thorough documentation (especially on the matter of checking episode boundaries, which tripped up me on my first read, as well as a few other people on the mailing list it looks like).\r\n\r\nBehavior is verified to be identical and DataSet CPU time is about 20% less (check the first changeset in this PR for the code I used to verify this). Total lines of code is about 115 less, including getting rid of the cython code to shift things around. Memory usage will be 9.09% less as well.\r\n\r\nOf course I'm open to any feedback or criticism. Let me know if the style isn't right for this project in some way. I may work on some of the other files next if you think this is an improvement."},"comment":{"url":"https://api.github.com/repos/spragunr/deep_q_rl/issues/comments/142603166","html_url":"https://github.com/spragunr/deep_q_rl/pull/45#issuecomment-142603166","issue_url":"https://api.github.com/repos/spragunr/deep_q_rl/issues/45","id":142603166,"user":{"login":"spragunr","id":4982892,"avatar_url":"https://avatars.githubusercontent.com/u/4982892?v=3","gravatar_id":"","url":"https://api.github.com/users/spragunr","html_url":"https://github.com/spragunr","followers_url":"https://api.github.com/users/spragunr/followers","following_url":"https://api.github.com/users/spragunr/following{/other_user}","gists_url":"https://api.github.com/users/spragunr/gists{/gist_id}","starred_url":"https://api.github.com/users/spragunr/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/spragunr/subscriptions","organizations_url":"https://api.github.com/users/spragunr/orgs","repos_url":"https://api.github.com/users/spragunr/repos","events_url":"https://api.github.com/users/spragunr/events{/privacy}","received_events_url":"https://api.github.com/users/spragunr/received_events","type":"User","site_admin":false},"created_at":"2015-09-23T13:36:06Z","updated_at":"2015-09-23T13:36:06Z","body":"This looks great!  It's a big improvement over the current implementation.  I wish I'd known about the take method when I originally coded this.  It would have saved a lot of trouble.  I'll merge it ASAP, but it may be a while before I get to it.  You could save me some time by making a couple of changes: \r\n\r\n* Keep line lengths below 80\r\n* Use explicit initialization of the instance variables in the constructor. The  `self.__dict__.update(locals())` trick doesn't really make the code much shorter, and it has the potential to confuse\r\n* update the dep script to remove the cython dependency. \r\n* update the README to remove the cython dependency. \r\n\r\nThanks!"}}