{"comment":{"url":"https://api.github.com/repos/conveyal/gridualizer/comments/20028924","html_url":"https://github.com/conveyal/gridualizer/commit/d06aa5eefe4433014391ba3f2a7bf47eff4af83c#commitcomment-20028924","id":20028924,"user":{"login":"abyrd","id":112871,"avatar_url":"https://avatars.githubusercontent.com/u/112871?v=3","gravatar_id":"","url":"https://api.github.com/users/abyrd","html_url":"https://github.com/abyrd","followers_url":"https://api.github.com/users/abyrd/followers","following_url":"https://api.github.com/users/abyrd/following{/other_user}","gists_url":"https://api.github.com/users/abyrd/gists{/gist_id}","starred_url":"https://api.github.com/users/abyrd/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/abyrd/subscriptions","organizations_url":"https://api.github.com/users/abyrd/orgs","repos_url":"https://api.github.com/users/abyrd/repos","events_url":"https://api.github.com/users/abyrd/events{/privacy}","received_events_url":"https://api.github.com/users/abyrd/received_events","type":"User","site_admin":false},"position":null,"line":null,"path":null,"commit_id":"d06aa5eefe4433014391ba3f2a7bf47eff4af83c","created_at":"2016-12-01T08:52:32Z","updated_at":"2016-12-01T08:52:32Z","body":"I'm reviewing/experimenting with the dot grid display code and have a few questions and comments. \r\n1. Why do we need to use a Halton sequence if we have a seedable pseudorandom number generator? I see you've got functions for both uniform random x and y and a Halton sequence, what was the difference in the result? As you noted, the Halton sequence implementation is (tail)recursive so probably slower than it would need to be. Comments on these functions saying that they are interchangeable position generators would be helpful in understanding the code.\r\n2. It took me quite a while to realize the positions variable was holding a function that generates positions on demand but repeatably for a given opportunity grid cell. Even short documentation about what's stored in a variable or what the signature of a function is can greatly help with later readings of the code. At first I assumed it was storing all the positions, which could be problematic since we'd easily end up with a 100MB lookup table. Seeding the generator with the density grid cell coordinates to keep the dots stationary between renderings or zoom levels is a good idea, I was in the midst of suggesting that when I realized you were already using the on-demand generator approach. \r\n3. The pixelWidthAtZoom factor is stored in a variable, then applied using normal divide and multiply operators in inner loops. I imagine the JS optimizer is not smart enough to realize that the result of Math.pow(2, x) is always a power of 2 and replace those divides with bit shifts. If this happens to be a hot spot, just storing the difference in zoom levels and scaling with bit shift operators could be faster (after all, this optimization is the reason each zoom level is 2x bigger than the previous one).\r\n4. The placement function is initializing and using a rather heavy-duty Mersenne Twister number generator and several multiply operations in a tight loop. There are very fast non-secure random number generators that might be better suited, e.g. https://en.wikipedia.org/wiki/Xorshift. \r\n5. The dot locations for all opportunities in a cell are still pre-calculated and stored in an array. This might be a good place to use an ES6 generator or generator iterator and produce them on the fly.\r\n6. You could use 8 bits of the random number generator's output directly as the pixel index (0...255) for a dot. Getting even more optimization-crazy, note also that the random number generator will produce 32 or 64 random bits at once, so you can get x and y coordinates for two or four dots at once from a single random number. This could be achieved by putting multiple yield statements in a the ES6 generator, each one performing a 1-byte shift."}}
{"actor":{"display_login":"abyrd"}}