Coot 12, refactor, arma::min, arma::imbue and some other bugs

[shrit]

Hi,

There is still one single test that is failing and I am not able to figure out where the issue is coming from. Here is the test failure:

./bin/mlpack_test
mlpack version: mlpack git-a62b12d4ed
armadillo version: 12.6.1 (Cortisol Retox)
random seed: 0
terminate called after throwing an instance of 'std::logic_error'
  what():  as_scalar(): expression must evaluate to exactly one element

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
mlpack_test is a Catch v2.13.10 host application.
Run with -? for options

-------------------------------------------------------------------------------
OneStepQLearningTest
-------------------------------------------------------------------------------
/meta/mlpack/src/mlpack/tests/ann/async_learning_test.cpp:21
...............................................................................

/meta/mlpack/src/mlpack/tests/ann/async_learning_test.cpp:21: FAILED:
due to a fatal error condition:
  SIGABRT - Abort (abnormal termination) signal

===============================================================================
test cases:     204 |     203 passed | 1 failed
assertions: 1070829 | 1070828 passed | 1 failed

[3]    3152935 IOT instruction (core dumped)  ./bin/mlpack_test

The issue here is that there are two as_scalar that are left in methods that could be used here, and both of them are being used correctly, (evaluate a vector and return a scalar).

I am also pretty sure that in a lot of places we can remove arma::as_scalar since it is used for no reason

[shrit]

@rcurtin I think we need to upgrade armadillo version in here

[shrit]

@rcurtin this is good from my end, I am not adding anything here.
Let us see if we can merge it before the weekend

[rcurtin]

Two really minor comments, otherwise, looks good to me. Sorry the review took so long 👍

[rcurtin]

I think that this shouldn't be needed---I believe if the input dimensions are left unset, the input is assumed to be a 1D tensor with number of elements equal to the number of rows in the matrix. Did you find that you needed to add this line, or will the test work without it?

[shrit]

The test was failing without it, so I have no idea, the dimensions were set up manually everywhere else.
Technically you are right, it is not necessary, but at the same time, the test was failing.

[rcurtin]

I tried to reproduce this locally, and I found that I could make a test failure happen with or without the code. It also doesn't look as simple as adjusting tolerances, I think there may be an invalid memory access in a different test that sometimes affects this one. Hopefully we can solve that, but here is not the place... up to you whether you want to remove this line or keep it there. 👍

[rcurtin]

Awesome, thanks for the fixes 👍

[rcurtin]

I tried to reproduce this locally, and I found that I could make a test failure happen with or without the code. It also doesn't look as simple as adjusting tolerances, I think there may be an invalid memory access in a different test that sometimes affects this one. Hopefully we can solve that, but here is not the place... up to you whether you want to remove this line or keep it there. 👍

[shrit]

okay, I will remove it tomorrow morning

[mlpack-bot]

Second approval provided automatically after 24 hours. 👍