{"action":"created","issue":{"url":"https://api.github.com/repos/schmie/test/issues/134","labels_url":"https://api.github.com/repos/schmie/test/issues/134/labels{/name}","comments_url":"https://api.github.com/repos/schmie/test/issues/134/comments","events_url":"https://api.github.com/repos/schmie/test/issues/134/events","html_url":"https://github.com/schmie/test/issues/134","id":90953584,"number":134,"title":"Make all streamers usable in new LB system","user":{"login":"schmie","id":6473158,"avatar_url":"https://avatars.githubusercontent.com/u/6473158?v=3","gravatar_id":"","url":"https://api.github.com/users/schmie","html_url":"https://github.com/schmie","followers_url":"https://api.github.com/users/schmie/followers","following_url":"https://api.github.com/users/schmie/following{/other_user}","gists_url":"https://api.github.com/users/schmie/gists{/gist_id}","starred_url":"https://api.github.com/users/schmie/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/schmie/subscriptions","organizations_url":"https://api.github.com/users/schmie/orgs","repos_url":"https://api.github.com/users/schmie/repos","events_url":"https://api.github.com/users/schmie/events{/privacy}","received_events_url":"https://api.github.com/users/schmie/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/schmie/test/labels/enhancement","name":"enhancement","color":"84b6eb"},{"url":"https://api.github.com/repos/schmie/test/labels/P:%20minor","name":"P: minor","color":"dcffff"}],"state":"open","locked":false,"assignee":{"login":"schmie","id":6473158,"avatar_url":"https://avatars.githubusercontent.com/u/6473158?v=3","gravatar_id":"","url":"https://api.github.com/users/schmie","html_url":"https://github.com/schmie","followers_url":"https://api.github.com/users/schmie/followers","following_url":"https://api.github.com/users/schmie/following{/other_user}","gists_url":"https://api.github.com/users/schmie/gists{/gist_id}","starred_url":"https://api.github.com/users/schmie/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/schmie/subscriptions","organizations_url":"https://api.github.com/users/schmie/orgs","repos_url":"https://api.github.com/users/schmie/repos","events_url":"https://api.github.com/users/schmie/events{/privacy}","received_events_url":"https://api.github.com/users/schmie/received_events","type":"User","site_admin":false},"milestone":{"url":"https://api.github.com/repos/schmie/test/milestones/17","html_url":"https://github.com/schmie/test/milestones/0.1","labels_url":"https://api.github.com/repos/schmie/test/milestones/17/labels","id":1177582,"number":17,"title":"0.1","description":"None","creator":{"login":"schmie","id":6473158,"avatar_url":"https://avatars.githubusercontent.com/u/6473158?v=3","gravatar_id":"","url":"https://api.github.com/users/schmie","html_url":"https://github.com/schmie","followers_url":"https://api.github.com/users/schmie/followers","following_url":"https://api.github.com/users/schmie/following{/other_user}","gists_url":"https://api.github.com/users/schmie/gists{/gist_id}","starred_url":"https://api.github.com/users/schmie/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/schmie/subscriptions","organizations_url":"https://api.github.com/users/schmie/orgs","repos_url":"https://api.github.com/users/schmie/repos","events_url":"https://api.github.com/users/schmie/events{/privacy}","received_events_url":"https://api.github.com/users/schmie/received_events","type":"User","site_admin":false},"open_issues":1,"closed_issues":38,"state":"closed","created_at":"2015-06-23T15:13:09Z","updated_at":"2015-06-25T12:47:08Z","due_on":"2011-11-03T00:00:00Z","closed_at":"2015-06-23T15:13:09Z"},"comments":5,"created_at":"2015-06-25T12:47:08Z","updated_at":"2015-06-25T12:47:29Z","closed_at":null,"body":"**Reported by hywel on 18 Oct 2011 09:57 UTC**\nIt seems cheating to get a 0.1 milestone without having all of the streamer code working. This was my fault, so I'm going to bring the FInterpolation, !GuoZhengShi and Regularised streamers up to scratch and write unit-tests for them. Simple bounce-back is being done under ticket 9.\r\n\r\nAssigning to Miguel for him to decide whether I should push it back to 0.2."},"comment":{"url":"https://api.github.com/repos/schmie/test/issues/comments/115240400","html_url":"https://github.com/schmie/test/issues/134#issuecomment-115240400","issue_url":"https://api.github.com/repos/schmie/test/issues/134","id":115240400,"user":{"login":"schmie","id":6473158,"avatar_url":"https://avatars.githubusercontent.com/u/6473158?v=3","gravatar_id":"","url":"https://api.github.com/users/schmie","html_url":"https://github.com/schmie","followers_url":"https://api.github.com/users/schmie/followers","following_url":"https://api.github.com/users/schmie/following{/other_user}","gists_url":"https://api.github.com/users/schmie/gists{/gist_id}","starred_url":"https://api.github.com/users/schmie/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/schmie/subscriptions","organizations_url":"https://api.github.com/users/schmie/orgs","repos_url":"https://api.github.com/users/schmie/repos","events_url":"https://api.github.com/users/schmie/events{/privacy}","received_events_url":"https://api.github.com/users/schmie/received_events","type":"User","site_admin":false},"created_at":"2015-06-25T12:47:29Z","updated_at":"2015-06-25T12:47:29Z","body":"**Comment by jamespjh on 27 Oct 2011 09:50 UTC**\nStreamers code review comments\r\n\r\nD3Q15.cc \r\n\r\nL255 const distribn_t* f should be \"const distribn_t * const f\" (you are changing neither where the pointer points to, nor the contents of the location. )\r\n\r\nL258 Consider moving the definition of the Cs to a static const member of the enclosing D3Q15 class - it is initialized from static const members, so should itself be same. Consider calling it discreteVelocityVectors instead of Cs.\r\n\r\nL273 It looks like the second loop could be rolled into the first, i.e. insert after L269\r\nif (jj!=ii) ret.tensor[= ret.tensor[jj](ii][jj])[consider abstracting this second loop into a symmetrise() method on 2-tensors.\r\n\r\nD3Q15.h\r\n\r\nL9 Consider making an inline operator[](ii];\r\nAlternatively,) method on the Order2Tensor class/struct, to avoid having to write mytensor.tensor.\r\n\r\nLocalLatticeData.cc\r\n\r\nL68 iSiteIndex and iDirection can be const.\r\nL74 Recommend insert a comment and break this line up - any bit-wise work can be a bug generator, so keep it really clear. I would comment as:\r\n\"Have a boundary in the specified direction if the corresponding bit of the boundary config is set.\"\r\nAnd would either define a local variable\r\nconst unsigned int directionMask=1U << (iDirection - 1)\r\nBetter still, define inline methods to encapsulate bit work: e.g. DirectionMaskFromDirection, BoundaryBitsFromSiteData.\r\nI also think an early return would be more readable than the if-else loop, i.e. begin the method with:\r\nif (iDirection==0) return false;\r\nL78 Needs a comment: \"iDirection==0 corresponds to movement to self, never a boundary in this direction.\"\r\nThis method could do with a few unit tests if it doesn't have them already.\r\n\r\nBaseStreamer.h\r\nL61 Worth a comment somewhere as to why the explicit \"template\" here (when calling a metaprogrammed method of the CRTP implementor is needed.)\r\n\r\nFInterpolation.h\r\n\r\nL29: iLbmParams I think can be const pointer to const instead of volatile pointer to const. (by the way, LbmParams is carried quite a long was as a chained observer parameter - via Collisions::Normal to the Kernels - this is often a sign that something can be refactored.) \r\nL30: bLatDat can be const pointer to non const. (geometry::LatticeData* const bLatDat)\r\nL35: Variable f : better variable name?\r\nParameter bLatDat : better variable name? \r\n(Remove hungarian b, i etc per new name convention?)\r\nL47: Chained assignment : suggest two separate assignments.\r\n\r\nL68ff: This is a massive improvement in readability! I approve hugely!\r\nL73: Suggest changing the index variable from l to direction\r\nAnd suggest insert a variable int inverseDirection=D3Q15::INVERSEDIRECTIONS[l] for further clarity.\r\n\r\nI haven't had time to go through each streamer in turn -- similar suggestions apply.\r\n\r\nunittests/FourCubeLatticeData\r\nL41 variable name ii should be collisionType\r\n\r\nunittests/lbtests/StreamerTests.h\r\nL131ff The new FInterpolation test is an integration test across the whole streamer behaviour. I'd still like to see more method-by-method unit tests.\r\nL138 Variable name ii->site\r\nL141 Variable name jj->direction\r\nL138-145 This \"initialize with anisotropic test data\" routine could be factored.\r\nI notice there is only a test for FInterpolation.\r\n"}}