{"action":"created","issue":{"url":"https://api.github.com/repos/schmie/hemeMove/issues/42","labels_url":"https://api.github.com/repos/schmie/hemeMove/issues/42/labels{/name}","comments_url":"https://api.github.com/repos/schmie/hemeMove/issues/42/comments","events_url":"https://api.github.com/repos/schmie/hemeMove/issues/42/events","html_url":"https://github.com/schmie/hemeMove/issues/42","id":94117753,"number":42,"title":"My work since pushing was banned","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":[],"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":null,"comments":14,"created_at":"2015-07-09T18:21:58Z","updated_at":"2015-07-09T18:22:38Z","closed_at":null,"body":"**Reported by nick on 20 Sep 2011 16:11 UTC**\nI'm not sure at what point I last pushed my changes, but now that I've implemented depth cuing and surface normal highlighting, it seems a good time to request a code review.\r\n\r\nThe code is in ~Nick/hemelb. Please use revision 5b24c4caba91 \r\n\r\nCheers,\r\n\r\nNick"},"comment":{"url":"https://api.github.com/repos/schmie/hemeMove/issues/comments/120094330","html_url":"https://github.com/schmie/hemeMove/issues/42#issuecomment-120094330","issue_url":"https://api.github.com/repos/schmie/hemeMove/issues/42","id":120094330,"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-07-09T18:22:38Z","updated_at":"2015-07-09T18:22:38Z","body":"**Comment by hywel on 29 Sep 2011 11:40 UTC**\nReplying to [nick](comment:13):\r\n> Replying to [hywel](comment:12):\r\n> \r\n> > Maybe this is the problem then - using gdb, you can normally find and fix such bugs in the  >same amount of time. If you want, we can sit down together some time and talk about how to use > gdb effectively, and I can show you how it works. Some people also recommend getting someone else to read through your code - Rupert found a lattice-Boltzmann bug (which I'd introduced) for me within about 2 minutes.\r\n> \r\n> > Amongst reasons to not use assert statements are that they don't diagnose or output what's wrong in the way logging does, they don't tell you anything about the root cause, they clutter the code with statements about what you didn't intend it to do (not necessarily what it's not able to do) and they kill the programme on every error (imagine a production scenario where we'd enable something like assertions to ensure our I/O was successful and that there were no injections in the HemeLB parameters and other stuff - the programme would also die if the image went too far blue which really isn't that big a problem).\r\n> \r\n> I'd be very keen to learn more about GDB but since I'm leaving on Friday, it might not be a good use of your time. I would be surprised if you can find bugs before you even began looking for them using GDB which assertions will do. I would agree that they don't tell you the root cause. I'm afraid I don't understand how logging will be any better. The assertions state the programmer's assumptions ie, I assume the inputs are positive, or I assume that the function will return a positive value, and are even better than comments because they're guaranteed to be true or the programmer will be informed otherwise. Killing the application is desirable - in your case, if the IO fails and there's no code to handle the failure of the IO, anything the application outputs is potentially invalid at this point. \r\n\r\nYou're probably right that it's too late to really get into GDB. I suppose my point here is that some programmers' assumptions are inviolable (e.g. the IO case I mentioned) and some aren't (e.g. hue going above 360.0) and assertions don't let you distinguish between cases. Using the logger stuff we have lets you specify the output a user gets and gives you the choice of killing the programme or not.\r\n\r\n> Nevertheless, since I'm leaving Friday and arguing is futile, I'll remove all the assertions. \r\n\r\nArguing certainly isn't futile; there's not been a firm decision made about assertions yet. I remain unconvinced and favour logging, Miguel is in favour of assertions and we're going to meet and make a decision sometime soon. If you don't want to replace your assertions with logging at the right level then it's probably better to leave them than remove them altogether.\r\n \r\n> > The GeometryReader was having problems in production runs that we didn't get on smaller local runs, so now has code that is only run on a certain logging level that tests all of our assumptions about our reading of the input file, and displays a human-readable error message when appropriate. You could do something similar here if you don't want to face debugging in gdb.\r\n> > \r\n> > > - The diff is prior to reformatting using eclipse, for clarity. \r\n> > \r\n> > Then it's not ready for review!\r\n> \r\n> What I meant is that the code has been reformatted by eclipse, but this would muddy the diff massively.\r\n\r\nAh OK. If you run diff with -bwB it should ignore most white-space changes.\r\n\r\n> > > XYCoordinates.h\r\n> > >   * Couldn't this whole class be combined with Vector3D to make a more\r\n> > >  general template<int dimension> template <typename T> Vector?\r\n> > >   - Probably, if you're willing to use a union *shudders* or auto\r\n> > >     generated code other than with templates \r\n> > \r\n> > Really? Couldn't you just have ```T xs[instead of ```T x, y, z;```\r\n> > You'd have to get rid of the access with a dimension enumeration, instead using a dimension \r\n> > int.\r\n> I see - switch to an array but then use a function accepting an enum to maintain readability in the cases where the dimension is referred to by letter? I can implement this if you want.\r\n\r\nYou can use an enum or just use an index integer.\r\n\r\n> > These changes aren't about how they compile, they're about explaining to another programmer what your code does (cf. Literate Programming). This code is a useful little class that isn't specifically about visualisation, therefore it belongs in the util namespace.\r\n> Fair point. If you want to go with above, the class will soon vanish. Otherwise I'll do this.\r\n\r\nI'd prefer the above one.\r\n\r\n> > >  ----\r\n> > >  HSLToRGBConverter\r\n> > > \r\n> > >   * The method seems quite over-engineered, and there are few comments.\r\n> > >  It's not clear to me why we can't use floats, nor why we have to specify\r\n> > >  how many bytes each int is. The wiki page you link to makes the conversion\r\n> > >  look much simpler.\r\n> > >   - Floats can be used, but unsigned integers have been preferred for\r\n> > >     more even precision. Sizes have been specified since C++ gives no guarantees\r\n> > >     about the size of types.\r\n> > >   - All the maths carried out in the code matches that on the wiki page\r\n> > \r\n> > No doubt, but if you choose to change it this much, you're going to need to add a lot more comments to explain what you're doing.\r\n> I'll get onto this.\r\n> \r\n> > >   !RayDataEnhanced\r\n> > >   * Do we definitely want to do the \"fabs(lDotProduct)\"? Have you tried\r\n> > >     versions where the back wall of a vessel isn't displayed?\r\n> > >   - Looking down the tube is no longer possible with the diffTest, and the end of the tube doesn't show up.\r\n> > >     Obviously the rest of the tube looks fine, but assymetric blood vessels won't look as 3D. Is an \r\n> > >     extra if statement really going to be cheaper than fabs and a multiplication? \r\n> > >     I realise fabs isn't quite as efficient as changing the sign bit as it does some checks for special values,\t\t        but I presume the branch prediction means that the hit is tiny.\r\n> > \r\n> > Eh? Why is looking down the tube impossible? Re: if  / fabs, it depends on ratio between how many times each is called. My point here is simply that your code treats towards- and away-facing surfaces the same and I wasn't sure if that was intentional.\r\n> \r\n> When you \"look down the tube\", you only see back walls. Therefore everything becomes maximum lightness. Ok - you're still looking at a projection of data within the tube, but it's impossible to perceive that this is the case. \r\n> \r\n> > I think we already have functionality for clicking on a pixel and getting its velocity. No idea if it's currently working. At the moment, I don't see us removing RayDataNormal until we're sure it's totally useless and can't even be modified to be useful.\r\n> There is code for density and stress - don't think it's implemented in VizClient.\r\n> \r\n> > The normal argument is that using pointers is always explicit - you always know you are doing stuff to a pointer, but I suppose most of us are using decent IDEs now so we can easily see the type of the variable and it's not a problem.\r\n> I don't think I understand. Do you mean that using a pointer means you're constantly reminded by the -> that the object may not actually exist or exists outside the scope of the function (unless someone takes an address of an object local to the function)? \r\n\r\nIt's more that using -> makes it obvious to the programmer that they are dealing with access to a non-locally-owned variable and changes could affect other bits of code. Using . for access to what is effectively a pointer doesn't cue the programmer in the same way (unless they're using an editor that tells them the variable is a reference rather than a local / member). But I think the pluses of references (non-NULL, no reassignment) probably outweigh the negatives.\r\n\r\n> > >   * Lots of long parameter names, these will look nicer if you can think of\r\n> > >  shorter versions (not always easy / possible).\r\n> > >   - I thought the same, but I can't think of shorter versions. The long ones\r\n> > >     are vectors, and describing the two ends of the vector is wordy.\r\n> > \r\n> > lLowerSiteToFirstRayClusterIntersection -> lowSiteToFirstClusterCrossing or something?\r\n> I'd be worried that it's not clear what a \"ClusterCrossing\" is or what a \"lowSite\" is. Of course lower site is already confusing - could be renamed to SiteOfLowestXYAndZCoordinates but at least lower site is used elsewhere in the raytracer.\r\n\r\nThe name doesn't need to say everything, it really needs to be concise and sufficiently accurate. If you need to explain in greater detail what your storing, use a comment where you declare the var.\r\n\r\n> > >   * Some of these functions could be 'const' - that helps the compiler\r\n> > >     optimise. [citation needed](numDims]```)\r\n> > >   - None of the public functions can be declared const, so IMO there's no need to \r\n> > >     declare the private/protected functions const, even if they could legitimately\r\n> > >     be declared const.\r\n> > \r\n> > It's a good hint to a compiler and it's a good hint to a programmer.\r\n> Fortunately, when I declared a function const which altered a member variable, the compiler knew better than to believe my hint. Isn't this \"feature\" of C++ because const isn't a separate part of the language but part of the static type system and there are some cases where the compiler can't tell if a function really is \"const\", just certain cases it clearly isn't \"const\"? \r\n> \r\n> Nevertheless, if you really want, I can go through and add const to the relevant private functions.\r\n\r\nConst tells me something important about the function without having to look at the implementation. The compiler knows when it reads the .cc and you've lied about the const-ness, I believe it also can effect optimisations being made or not when using functions with pointers (const on one of the pointers will imply that the pointers you've passed don't overlap or something). This probably doesn't affect your case, but the readability point remains (which for a long-maintained code is nearly as important). See http://www.parashift.com/c++-faq-lite/const-correctness.html#faq-18.3 if unconvinced.\r\n\r\n> > We use ```(type)```-style casts in the rest of the code, which is the more important sort of consistency. They're also clearer.\r\n> I'm not sure what you mean by clearer, and I'm surprised you use the backwards compatibility of C++ over it's native casts, but I'll replace them for consistency.\r\n\r\nI don't think there's any difference in the generated assembly but concise code is always more readable (especially if it means you can fit one statement on one line).\r\n\r\n> > >  !ClusterShared\r\n> > >  * !DoGetBlockIdFrom3DBlockLocation repeats an assumption that's in other\r\n> > >  places (like the !LatticeData), which should be combined somewhere.\r\n> > >  - True, but the only similarities are that they're 3D block structures. If you want\r\n> > >    I could create a common (perhaps templatable) class for this, for both\r\n> > >    !ClusterShared and !LatticeData.\r\n> > \r\n> > Whatever is similar about them should be combined. The concept that {x,y,z}->index has x co-ordinate as most significant and z co-ordinate as least should ideally only appear once in our whole code.\r\n> \r\n> Do you want me to create a new templated type containing a T*** with some nice methods and using this in all cases? If so, what do you want it called, wand where do you want it placed? \r\n\r\nThat sounds like a good solution. It sounds like it belongs in utils, maybe called ```template<typename LatticeType> Lattice3D;``` .\r\n\r\n>  \r\n> > >  ----\r\n> > >  !ClusterTraverser\r\n> > >   * This should keep a pointer\r\n> > >   - Again, null case not valid and no pointer arithmetic \r\n> > >   * Would it be useful to have a method to return info about the current\r\n> > >     location in the cluster? Would that simplify some of the classes that use\r\n> > >     it?\r\n> > >   - These are inherited. \r\n> > \r\n> > I can't see the code, but I meant a method like !GetWallData(int index) instead of doing !GetLocation(int index) and then calling code getting the wall data.\r\n> I think I understand. The ClusterTraverser works on a block level. The ClusterTraverser could possibly return a inherited version of SiteTraverser which could be used in this manner. I think this would be less simple than simply obtaining the block index and using this as an argument to TraverseVoxel. \r\n\r\nIf you think it'd be less simple, no worries.\r\n\r\n> \r\n> > You've replaced the original diff with an interdiff. Next time, can you replace with a fresh diff please (or at least leave the original diff)? Ideally of all the non-merge changes since your latest common ancestor with entropy. Ta.\r\n> I never uploaded the original diff as it was polluted by all the merges. I assumed that last time you did the code review, you simply looked at the files in my repository? \r\n\r\nYup, you're totally right. Sorry about that."}}