Conversation
|
Okay, great so +409 issues, I think I fixed most of the issues, but the question is, is a single commit fine, or should we split the fixes e.g. one commit for each file/method, etc.? |
|
One commit is fine with me, mostly I'm just going to scroll through the combined PR diff in Github and see if anything seems out of place. |
|
Okay, I'm not sure what the issues are with |
|
Not sure what we should do with the |
rcurtin
left a comment
There was a problem hiding this comment.
Just a few comments, looks good otherwise.
| public: | ||
| template <typename LayerType> | ||
| LayerTypes operator () (LayerType* ) const; | ||
| LayerTypes operator () (LayerType*) const; |
There was a problem hiding this comment.
I think in other places for operators we've done, e.g., operator=(args& args...), so I would expect here it might be operator()(LayerType*); is this a gray area in the style check rules?
There was a problem hiding this comment.
hm, good point, I'll take a look into the config.
| "set of points. You may specify a separate set of reference points and " | ||
| "query points, or just a reference set which will be used as both the " | ||
| "reference and query set. You must specify the rank approximation (in \%) " | ||
| "reference and query set. You must specify the rank approximation (in %%) " |
There was a problem hiding this comment.
Since this isn't being given to printf, I think that just % is fine here.
src/mlpack/tests/load_save_test.cpp
Outdated
| f << "@attribute @@@@flfl numeric" << endl; | ||
| f << endl; | ||
| f << "\% comment" << endl; | ||
| f << "%% comment" << endl; |
There was a problem hiding this comment.
Same here, I think one % is fine.
I've noticed that the style check only checked the source files in src/mlpack/core, this was fixed in mlpack/jenkins-conf@b63e8ae.