stan-dev / stan

Stan development repository. The master branch contains the current release. The develop branch contains the latest stable development. See the Developer Process Wiki for details.
https://mc-stan.org
BSD 3-Clause "New" or "Revised" License
2.56k stars 366 forks source link

update opencl assign test to reassign zero sized vector after recover_memory() #3219

Closed SteveBronder closed 9 months ago

SteveBronder commented 11 months ago

Submission Checklist

Summary

Fixes a bug in the stan opencl assign tests that reuses a var_value<matrix_cl<double>> after stan::math::recover_memory() is called. This popped up in https://github.com/stan-dev/math/pull/2905 in the math. My guess is that this could be the cause of why sometimes these tests would fail randomly.

@t4c1 if you have any time could you take a look at this? I wasn't sure if this was intended as some part of the test or not and I'm missing something. You can replicate the error this fixes with the following

git clone --recursive git@github.com:stan-dev/stan.git
cd lib/stan_math
git checkout feature/threadsafe-matrixcl
cd ../../
python ./runTests.py ./src/test/unit/model/indexing/assign_cl_test.cpp

It's odd because after recover_memory() is called m_empty_v_cl will have a size of (4, 1). Which is the same size as the m2_cl vector we try to assign to it, but that test successfully throws so I'm not sure why that would happen.

Intended Effect

Fix bug in var_value<matrix_cl<double>> assignment

How to Verify

python ./runTests.py ./src/test/unit/model/indexing/assign_cl_test.cpp

Side Effects

Documentation

Copyright and Licensing

Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company): Simons Foundation

By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses:

t4c1 commented 11 months ago

Sorry, it has been too long since I worked on this. The code does not ring any bells. But from a cursory glance this change looks good.

SteveBronder commented 9 months ago

@rok-cesnovar could you look at / approve this?

rok-cesnovar commented 9 months ago

Of course, on it.