backported ind2sub and sub2ind. #683
Conversation
| { | ||
| arma::mat A = arma::randu(5,5); | ||
| arma::uvec u = arma::ind2sub(arma::size(A), 3); | ||
| u.print(); |
There was a problem hiding this comment.
Hmm, this test doesn't actually test anything in the code, do you think that we can test the output with BOOST_REQUIRE_EQUAL? I don't know what the output values of u are supposed to be, but maybe something like
BOOST_REQUIRE_EQUAL(u(0), 0);
BOOST_REQUIRE_EQUAL(u(1), 3);
Technically I don't think we need to test Armadillo support since Armadillo should already be testing that, but, one nice thing this test does do is ensure that ind2sub() compiles correctly. If you like, we can also add a very simple test like this for sub2ind().
|
Now the test checks whether ind2sub and sub2ind work as they are supposed to. |
|
Why did travis show this totally unrelated error? |
|
Travis runs all of the tests, and some of the mlpack tests are probabilistic so they have a nonzero chance of failing. We try to keep the probability of failure acceptably low but sometimes we have not kept it low enough so sometimes Travis builds a test and it is unlucky and fails. I generally only care about Travis output insofar that it builds successfully and the relevant tests to the PR are passing. |
| // images (3), boundaries (1), segmentations (1). | ||
| int loop_iter = num_images * 5; | ||
| size_t row_idx = 0; | ||
| int col_i = 0, col_s = 0, col_b = 0; |
There was a problem hiding this comment.
use size_t maybe is a better choice, since this type guarantee it is large enough for the index of array. Besides, if the value would not change in the future, I think they are good candidates to become const
| rgb2xyz(0,0) = 0.430574; rgb2xyz(0,1) = 0.430574; rgb2xyz(0,2) = 0.430574; | ||
| rgb2xyz(1,0) = 0.430574; rgb2xyz(1,1) = 0.430574; rgb2xyz(1,2) = 0.430574; | ||
| rgb2xyz(2,0) = 0.430574; rgb2xyz(2,1) = 0.430574; rgb2xyz(2,2) = 0.430574; | ||
|
|
There was a problem hiding this comment.
I guess this one is easier to read and write
arma::mat rgb2xyz(3,3);
rgb2xyz = 0.430574;
Are you sure all of the value of the matrix are same?
There was a problem hiding this comment.
No values are not same, i forgot to initialize them. i will correct it.
|
I read all of the codes before Gradient, write down some opinions and ideas, you can take it as reference if you like. Will read the other part on next week. Thanks for the contributions, this class is quite hard to implement. |
|
You're welcome. I'll try to incorporate the changes you suggested along with the tests for these functions and file an updated PR as soon as possible. Thank You for your analysis. :) |
|
I wonder why all the comments came to this PR but none of the changes that were being commented on were part of this PR? I guess once a new PR is filed this will not be a problem. :) |
Oh my god, you are right. The pull request was revert already..... Installing ubuntu on my old laptop, I hope mlpack could be quite easy to build on ubuntu. |
No description provided.