search

stan-dev / math

The Stan Math Library is a C++ template library for automatic differentiation of any order using forward, reverse, and mixed modes. It includes a range of built-in functions for probabilistic modeling, linear algebra, and equation solving.
https://mc-stan.org
BSD 3-Clause "New" or "Revised" License
748 stars 187 forks source link

test/unit/math/prim/mat/functor/num_threads_test.cpp #1381

Closed alashworth closed 5 years ago

alashworth commented 5 years ago

All tests in test/unit/math/prim/mat/functor/num_threads_test.cpp fail when STAN_THREADS is not defined.

For example, the first test goes as follows:

set_n_threads("10");
EXPECT_EQ(stan::math::internal::get_num_threads(100), 10);

The expected value is 10. The actual value is 1.

By default, get_num_threads returns 1 when STAN_THREADS is undefined. A similar argument applies to the rest of the assertions in this file.

Current Version:

v2.20.0

alashworth commented 5 years ago

This is a little out of scope for this bugfix issue, but it would be helpful if all tests that need special compilation flags were isolated in their own directory. Like OpenCL is moving to. IMO, unit tests which are testing some functionality of Stan should be located close together in the file system.

The alternative, which is what I'm currently doing in CMake, is to go through and mark each test by hand in the build script if it should be conditionally compiled. I guess what is currently done is that every configuration of Stan gets the whole suite of tests recompiled, but this doesn't scale well at all, leading to a combinatorial explosion of unit tests.

wds15 commented 5 years ago

I see. I am right now changing the function to work under any circumstance the same way... and we actually do test sunsets under specific conditions. Have a look at the windows threading tests in Jenkins, for example. Often we do test indeed the full combinatorics though and that does not scale well....yes.

alashworth commented 5 years ago

Should this issue be closed with the TBB merge?

rok-cesnovar commented 5 years ago

Yes, this test no longer exits. Thanks for the reminder. Closing.