Repository navigation
Flatten mix/fun and finish flattening of /mix - #1584
Conversation
|
There's the old |
|
What is weirder is that the test error seems to be 1. I ran this on my Ubuntu setup with mpi, exact same compiler, and the test passes. This PR only moves the test so this is very bizarre. Will dig into this test a bit more. |
That is a good point also yes. |
|
Ah, I found it. This was probably it all along. Only |
|
Well spotted! |
mcol
left a comment
mcol
left a comment
There was a problem hiding this comment.
I couldn't find anything that needs changing, so it's ready to go!
|
This is not having an easy time: That's on Windows, no issue on Linux |
|
I've reproduced it on Windows locally, and it still causes an error in the compiler, but the traceback is slightly different: |
|
Does this bug show up only after flattening? And it's not a runtime error, but the compiler segfaulting? We had problems with windows before with the linker running out of memory and crashing, but that was for the language and we refactored it into more translation units to solve the problem. Are bigger translation units getting compiled after the refactor?
… On Jan 5, 2020, at 3:43 PM, Marco Colombo ***@***.***> wrote:
I've reproduced it on Windows locally, and it still causes an error in the compiler, but the traceback is slightly different:
In file included from ./stan/math/fwd/functor.hpp:4:0,
from ./stan/math/fwd.hpp:14,
from ./stan/math/mix.hpp:9,
from ./test/unit/math/test_ad.hpp:8,
from test/unit/math/mix/fun/log_mix_test.cpp:1:
./stan/math/fwd/functor/gradient.hpp: In function 'stan::test::internal::expect_ad_helper(const stan::test::ad_tolerances&, const F&, const G&, const VectorXd&, Ts ...)::<lambda(int)>::<lambda(const auto:5&)>
[with auto:5 = Eigen::Matrix<stan::math::fvar<double>, -1, 1>; F = mathMixScalFun_logMix_Test::TestBody()::<lambda(const auto:46&, const auto:47&, const auto:48&)>; G = stan::test::internal::expect_ad_vvv(const stan::test::ad_tolerances&, const F&, const T1&, const T2&, const T3&)
[with F = mathMixScalFun_logMix_Test::TestBody()::<lambda(const auto:46&, const auto:47&, const auto:48&)>; T1 = double; T2 = int; T3 = double]::<lambda(const auto:18&)>; Ts = {double, int, double}]':
./stan/math/fwd/functor/gradient.hpp:47:31: internal compiler error: Segmentation fault
fvar<T> fx_fvar = f(x_fvar);
^
Please submit a full bug report,
with preprocessed source if appropriate.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
Sorry about that---it's probably my bad in the original version. There were a few other ones of these that people found---I guess it was just making random tests under Mac.
… On Jan 5, 2020, at 9:23 AM, Rok Češnovar ***@***.***> wrote:
Ah, I found it.
Eigen::VectorXd v5(5);
v5 << 1, 2, 3, 4, 5;
Eigen::VectorXd v5b(5);
v5 << 2, 3, 4, 5, 6;
Eigen::VectorXd rv5(5);
v5 << 1, 2, 3, 4, 5;
Eigen::VectorXd rv5b(5);
v5 << 2, 3, 4, 5, 6;
stan::test::expect_ad(tols, f, v5, v5b);
stan::test::expect_ad(tols, f, rv5, rv5b);
This was probably it all along. Only v5 is initialized. Will fix this in this PR. subtract_vec_mat suffers from the same problem.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
Commenting out the At |
|
On Jan 5, 2020, at 7:32 PM, Marco Colombo ***@***.***> wrote:
Commenting out the matMixscalFun test block solves the segfault (as the file becomes the one before flattening), while commenting out the mathMixMatFun test block makes no difference.
For me it's enough to comment out any one of the stan::test::expect_ad lines in the
void expect_log_mix(const std::vector<double>& p, const std::vector<std::vector<double>>& ds) function to solve the bug. Both when it fails and when it succeeds, I see memory use for cc1plus.exe going over 4.1GB (I have 16GB).
That's a relief that it's a resource issue, not a problem with our code. Given the behavior outlined above, I'd think breaking the test down into multiple tests would solve this problem.
At -O2 it still crashes, but at -O1 it compiles fine. When setting the optimization level to -O0 I got:
That's probably because it's including debug output---that can be controlled separately from optimization level, I think.
|
Yeah, log_mix_test was one of the rare ones in this flatten where there were tests in multiple subfolders (scal and arr in this case) so this PR merges the scalar tests with the existing array tests. It seems that the one added test in this file caused it to crash due to lack of resources.
Thanks so much for debugging this one! Will probably split this in multiple files if there is no other non-hacky solution to this problem. @alashworth Do you have any tricks up your sleave to handle compiling larger test files on Windows? Since you are using jumbo builds you probably already seen this before. |
|
The "easiest" fix is to make sure you are compiling with |
|
Thanks, trying if seting the flag will work. It doesnt seem as huge of a test file to be needing other tricks: https://github.com/stan-dev/math/blob/5e18d5eb189008175178bd207a8453dbbc152b49/test/unit/math/mix/fun/log_mix_test.cpp Hopefully :) |
|
The only solution we've found in the past is smaller translation units.
… Thanks so much for debugging this one! Will probably split this in multiple files if there is no other non-hacky solution to this problem.
|
|
Yeah, the produced the same seg fault, so multiple translation units it is. @bob-carpenter Should I just split them to log_mix_arr_test.cpp and log_mix_test_scal_test.cpp? |
|
@bob-carpenter Should I just split them to log_mix_arr_test.cpp and log_mix_test_scal_test.cpp?
That's OK (dropping the double "test" in the scal version) or you could even use log_mix_part1_test.cpp and log_mix_part2_test.cpp to indicate they're part of the same test in some sense.
|
This reverts commit 5e18d5e.
|
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 1.0) |
|
I restarted the test with the Stan upstream pointing to your PR @mcol. We should hit green now. |
|
I pushed a fix for an include that I introduced on develop, and restarted the job pointing to stan-dev/stan#2874. I hope it's alright! |
|
Sure. Thanks! |
|
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 0.97) |
5 participants
Summary
This PR flattens mix/fun and finishes flattening the mix folder by creating the stan/math/mix.hpp header. This again touches a lot of files, but the changes are very simple. Only around 5 files required manual merging, the rest are renaming changes.
Tests
This also fixes #1453 with 744fea1
Side Effects
/
Checklist
Math issue Flatten scal / arr / mat #937
Copyright holder: Rok Češnovar
The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
- Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
- Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)