Hi, Can anyone tell what has happened to Linux builds?

I think what you have seen is related to: https://azure.microsoft.com/en-us/blog/our-commitment-to-customers-and-microsoft-cloud-services-continuity/

Hey @mrityunjay-tripathi, @favre49, @zoq Do you think there should be a way to test this ?
i.e. We can test it for a float datatype (or any other datatype) so that we can ensure everything is perfect but more importantly also show a new contributor or someone who likes looking at mlpack code why we did this (since this aims to solve #2338).
Testing generally makes me feel safer with the code that gets merged. I hope this is okay. Let me know what you think. Thanks a lot.

Hey @mrityunjay-tripathi, @favre49, @zoq Do you think there should be a way to test this?
i.e. We can test it for a float datatype (or any other datatype) so that we can ensure everything is perfect but more importantly also show a new contributor or someone who likes looking at mlpack code why we did this (since this aims to solve #2338).
Testing generally makes me feel safer with the code that gets merged. I hope this is okay. Let me know what you think. Thanks a lot.

Sure @kartikdutt18. Testing for the precision of all the loss functions is going to be a lot of work. But the currently added test of loss functions doesn't have even precision sufficient for float type, leave alone double. Or if you are saying just to test if we are getting correct return type or not? We can check that just by using a single line: typeid(x).name() or by checking the precision of returned value by using something like std::numeric_limits<InputType::elem_type>::max_digits10. The point is if the user feeds in float type matrix as input and target, float value will be returned by armadillo. This seems a thing more of armadillo to me. I'm still trying to figure out what kind of test can make sense?

I don't think we need to add tests for all layers. A single test to show that various data types like float can be used as both input and output. Let me know what you think.

Hey @mrityunjay-tripathi, I looked around and I found this issue #506, So this what I think you can do. In loss function tests use arma::fmat in some cases (not all so that we preserve double as well). We need not check for precision or datatype of output. If this works correctly, which I think it does it will return the correct output. The way we will test is by using something similar to contradiction. Since arma::fmat shouldn't work with current loss functions but does after your PR then we can say that templating solved the issue. I hope this makes sense. @zoq, @favre49, I think there might be a better way to test this, Could you help us out with the same. Thanks a lot.

It's good that builds are passing :)

Personally, I'm not a fan of the idea of testing float on some tests and double on others - it makes the tests more convoluted and confusing, and doesn't feel like good testing practice. To me, the point of these tests is ensuring the loss functions return the correct "answer". I agree with @mrityunjay-tripathi that the datatype of these tests feels like something that we should be trusting Armadillo on.

We can test it for a float datatype (or any other datatype) so that we can ensure everything is perfect but more importantly also show a new contributor or someone who likes looking at mlpack code why we did this

I'm not sure I understood this? Ideally the fact that this works should be obvious from documentation, not from tests

I'm not sure I understood this? Ideally the fact that this works should be obvious from documentation, not from tests

Generally I feel many people learn about the codebase by looking at the tests and why certain things are implemented they way are. Since what we had worked and we are adding another feature it made sense to me test the problem that it aimed to solve (in this case, using float datatype) before we merged it.
The way I would have tested it would be:

Create a separate Test Case, say LossFunctionTemplateTest and added the one or two loss function objects to show they worked on float or another datatype. This is what was done for Valid and Same padding options for convolution layers. Original tests weren't changed at all.
Sample Code:

/**
 * Simple test to show support for various datatypes supported by loss functions.
 */
BOOST_AUTO_TEST_CASE(LossFunctionTemplateTest)
{
   // Shows that loss functions support various datatypes including float.
   input = MatType("17.45 12.91 13.63 29.01 7.12 15.47 31.52 31.97");
   target = MatType("16.52 13.11 13.67 29.51 24.31 15.03 30.72 34.07");
   ElemType loss = module.Forward(input, target);
   BOOST_REQUIRE_CLOSE_FRACTION(loss, 2.410631F, 0.00001F);

   // We also support uint or any other datatype that is supported by armadillo.
   input = arma::ones<arma::umat>(10, 1);
   target = arma::ones<arma::umat>(10, 1);
   loss = module.Forward(input, target);

}

Maybe it's a problem with me, I consider testing to be absolutely necessary for any feature addition, but maybe it's unnecessary here. If you will feel that it is unnecessary, @mrityunjay-tripathi can use git revert to back to the previous state.
Thanks a lot.

That's okay. No test is unnecessary. Let's wait and see what other members think.

I like the general idea, in ensmallen, we run each test against arma::mat and arma::fmat. Here is an example: https://github.com/mlpack/ensmallen/blob/master/tests/ftml_test.cpp.

So ideally we can do the same here in mlpack, that includes everything, not only the ann methods. That said, I think it's out of scope for this particular PR. Also, we have to make sure the datatype used makes sense, like using arma::umat might work but the output is probably not what we want, without proper quantization.

So personally I would revert the change, the changes work with our main datatype right now, so it doesn't break anything.

I like the general idea, in ensmallen, we run each test against arma::mat and arma::fmat. Here is an example: https://github.com/mlpack/ensmallen/blob/master/tests/ftml_test.cpp.

This looks nice. It should not be hard to extend tests for fmat as well, I think we would be able to close #1062 when methods (other than ann) also support float and are tested. Do you mind if I open up an issue for new contributors to get involved with the codebase through testing or I can create a PR as well if this seems like something that we can add.

like using arma::umat might work but the output is probably not what we want, without proper quantization.

Agreed, that makes sense.

Till then @mrityunjay-tripathi, Kindly revert the changes and we can merge this. Hopefully tests for fmat etc. will follow soon. Sorry that you and @favre49, had to deal with me. Thanks a lot.

Thanks again, that is a good basis we can build on.