Replaced SortStruct by std::pair #721

Merged
merged 3 commits into from Jul 13, 2016

Projects

None yet

2 participants

@lozhnikov
Contributor

I replaced SortStruct by std::pair in the R* tree and the X tree and fixed some compiler warnings about uninitialized variables. I did some tests, std::pair provides some speedups in mlpack_knn for the R* tree and the X tree.

lozhnikov added some commits Jul 12, 2016
@lozhnikov lozhnikov Replace SortStruct by std::pair 640f3a7
@lozhnikov lozhnikov Fix compiler warnings
ee2a8a1
@lozhnikov lozhnikov commented on the diff Jul 12, 2016
src/mlpack/methods/ann/layer/one_hot_layer.hpp
@@ -58,7 +58,7 @@ class OneHotLayer
output = inputActivations;
output.zeros();
- arma::uword maxIndex;
+ arma::uword maxIndex = 0;
@lozhnikov
lozhnikov Jul 12, 2016 Contributor

I looked through the armadillo code, op_min::direct_min equates index_of_min_val to zero by default.

@rcurtin
rcurtin Jul 12, 2016 Member

Yeah; I agree that the -Wuninitialized is erroneous in this case, but I am not sure there is any other way to stop that warning from appearing here other than initializing the index to 0. Do you know any other ways?

@lozhnikov
lozhnikov Jul 12, 2016 Contributor

No, I don't. I think this is the easiest way.

@rcurtin rcurtin and 1 other commented on an outdated diff Jul 12, 2016
...mlpack/core/tree/rectangle_tree/x_tree_split_impl.hpp
}
}
- std::sort(sorted.begin(), sorted.end(), structComp<ElemType>);
+ std::sort(sorted.begin(), sorted.end(),
+ [] (const std::pair<ElemType, size_t>& p1,
+ const std::pair<ElemType, size_t>& p2)
+ {
+ return p1.first < p2.first;
+ });
@rcurtin
rcurtin Jul 12, 2016 Member

Do you think it would be better to define this comparator function in some header file in the rectangle_tree directory instead of writing the lambda many times? If not, I'll merge as-is, but it might be a good idea to make the code a bit shorter.

@lozhnikov
lozhnikov Jul 12, 2016 Contributor

I replaced lambdas by a comparator. I define this function in RStarTreeSplit and XTreeSplit. These classes have a lot of duplicated code, I think we should not create a separate file for the comparator.

@lozhnikov lozhnikov Replace lambdas by a comparator.
1938442
@rcurtin
Member
rcurtin commented Jul 12, 2016

Looks good to me. This fixes #721.

@rcurtin rcurtin closed this Jul 12, 2016
@rcurtin rcurtin added this to the mlpack 2.0.3 milestone Jul 12, 2016
@lozhnikov
Contributor

Do you mean #712? :)

@rcurtin
Member
rcurtin commented Jul 12, 2016

Yep, I must have mistyped... :)

@lozhnikov
Contributor

I mean to say that if the code looks good, did you forget to merge the PR?

@rcurtin
Member
rcurtin commented Jul 13, 2016

Oh, yes, so I not only mistyped, I also hit the wrong button!

@rcurtin rcurtin reopened this Jul 13, 2016
@rcurtin rcurtin merged commit 81c14d9 into mlpack:master Jul 13, 2016

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment