Refrain from using globals in Validation(). #495
Conversation
|
I don't see why continuous integration could fail on my changes here. It seems that fail occurs on installing torch dependencies but not my code. |
You're right - it doesn't look like a problem with your code. I've restarted the TravisCI build, let's see how it goes ... |
|
Sure enough, it passed. See torch/ezinstall#80. |
| end | ||
| end | ||
|
|
||
| local label_function = network.labelHook or function (input, dblabel) return dblabel end |
There was a problem hiding this comment.
sorry for being picky but the naming convention is slightly inconsistent with the other changes (where camel case is being used for globals)
There was a problem hiding this comment.
I did not get which is the consistent naming convention in this file because it was mixed inside the same Validation() function and in global variables. For example, the following names are used in the code: network_func, epoch_round, Weights, Gradients, trainBatchSize, etc.
I could make all of them consistent if only I knew which notation to use. Maybe in separate pull request?
There was a problem hiding this comment.
Indeed the trouble is that the naming conventions are inconsistent in the first place... I was just noting that you seemed to favor camel case notation for globals there and underscore notation for locals there. I think that's fine. However this change introduced a new global with underscore notation so I was thinking this wasn't entirely consistent... I am not suggesting that we fix all of these in this pull request but maybe if we could stick to one policy for new code that would be nice.
|
Thanks @Liuftvafas! Can you squash your commits and then I think the change will be good to merge. For the record, I tried moving Validation() out of |
|
Moving Validation() to separate file was really what I had to do for testing (but I didn't this time). Thanks. Just to clarify, do you suggest to squash all 4 commits from this pull request into a single one? |
|
Yes please, we usually squash commits into a single one before merging a pull request, unless several authors contributed. This allows keeping the commit history more concise. |
Use consistent naming of validation variables.
|
The change looks good to me, thanks! |
Refrain from using globals in Validation().
No description provided.