Fix miscompilation of array of tuples of complex numbers in data block

[WardBrian]

Closes stan-dev/stan#3437.

The logic to read in arrays-of-tuples from a var_context is quite complex, and the current code missed the subtlety that var context already merges complex numbers for us, so it was treating them as being offset-2 apart in the buffer rather than side by side.

Our existing test/integration/good/tuples/arrays-tuples-nested.stan test actually contained this bug, but it is only visible at runtime and that model is never actually instantiated with data in our tests.

Submission Checklist

• Run unit tests
• Documentation
  • If a user-facing facing change was made, the documentation PR is here:
  • OR, no user-facing changes were made

Release notes

Fixed an issue compiling data blocks with arrays containing tuples containing complex numbers, which would lead to mangled numbers or crashes.

Copyright and Licensing

By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the BSD 3-clause license (https://opensource.org/licenses/BSD-3-Clause)

[WardBrian]

There's also something else going on, this fixes the out of bound reads but the reconstructed values still aren't quite right

[SteveBronder]

Should I review this or wait until the reconstructed values fix is also done? I think I wrote the stan repo code for this and can take a look there

[WardBrian]

I'd wait, I think that var_context.vals_c is just fundamentally kinda broken for tuples because of the way it assumes the vals_r return is rectangular (I think the io format for complex numbers has caused more bugs than the io format for tuples at this point)

[WardBrian]

The latest push is closer (Jonah's example gets the right thing now) but wrong still for the case where it's a complex_vector(/matrix) nested inside of an array of tuples

[jgabry]

Thanks for working on this. I guess it's not super urgent since nobody has hit it apparently, probably because an array of tuples of complex numbers is quite rare. I only found it because I was intentionally stress testing fixes to complex number and tuple handing in cmdstanr and giving it all the possible combinations Claude and I could come up with.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.37%. Comparing base (af25f19) to head (dc6ea34).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1734   +/-   ##
=======================================
  Coverage   92.36%   92.37%           
=======================================
  Files          70       70           
  Lines       10545    10552    +7     
=======================================
+ Hits         9740     9747    +7     
  Misses        805      805           

Files with missing lines	Coverage Δ
src/middle/SizedType.ml	90.78% <100.00%> (-0.07%)	⬇️
src/stan_math_backend/Lower_expr.ml	96.89% <100.00%> (+0.01%)	⬆️
src/stan_math_backend/Transform_Mir.ml	95.70% <100.00%> (+0.04%)	⬆️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[SteveBronder]

Looking at the code here that seems wild both in complexity and readability.

https://github.com/stan-dev/stanc3/pull/1734/changes#diff-14fdde1929a5d96f22ddcb68717aa8821ceab63dc7f392ecff4b0e681dca3a35R650

I wonder if in the stan repo we can write a read_from_context(...) function that would handle all of the scary logic here in a way we could isolate and test better. I'll look into this

[WardBrian]

I have a change I didn’t push that I think fixes everything and cleans stuff up, I need to test it when I’m back in a week.

The generated code is a lot, but we’re also creating almost intentionally evil examples in the tests

[SteveBronder]

I have a branch here that has a read_from_context(...) function which I think would let us replace a lot of the big loops and logic the compiler generates. I think this would make the generated C++ a lot easier to read as well

[WardBrian]

I pushed my changes from before my week off but mostly for historical purposes, if we can use @SteveBronder's stan branch I think that would be universally better.

I need to write some more test models a-la the one @jgabry prepared. We can then put those as 'gold' tests in performance-tests-cmdstan stan-dev/performance-tests-cmdstan#72

[SteveBronder]

Should they be gold tests or should they be tests in the stan repo?

[WardBrian]

If we put them in performance tests cmdstan we would catch if the code-gen here broke them, which is nice. But I guess if most of the code gen moves to templated functions in stan, then the tests would make sense there too

[WardBrian]

I've started writing golds over at stan-dev/performance-tests-cmdstan#73

I think it's possible that we're also incorrectly handling 2-d arrays of tuples, though I'm having a hard time exactly wrapping my head around it

[SteveBronder]

@WardBrian I'm working on cleaning up the context reader branch and am going to open that today.

The API right now would look like the following the following in the generated stanc code

std::vector<...> user_data = // fill in data structure with NA values
validate_dims(...);
read_from_context(user_data, ctx, "user_data");

Internally the code detects tuples in user_data and handles them, but it leaves filling the original data structure with NA values and doing dimensionality checks to the compiler. I could modify the API so that we also handle the data structure setup and validation. But that will take me a little more time and I think we handle each chunk of this in pieces.

[WardBrian]

filling with NAs isn't too bad on the compiler side, anyway.

I think this branch and the tests added in performance-tests are correct now, so if we can replace it with something simpler while keeping those passing I'm all for it.

[SteveBronder]

I think it's possible that we're also incorrectly handling 2-d arrays of tuples, though I'm having a hard time exactly wrapping my head around it

Confirmed — Brian's instinct was right, and it's a real bug.

I'll add this to the stan PR

[WardBrian]

It's possible the bug was codegen-only, in which case the Stan PR may naturally do the right thing

https://github.com/stan-dev/performance-tests-cmdstan/blob/compiler-stress-tests/compiler-stress-models/onepl.stan is a really simple test model. If you provide this data file, the output should be 1 through 12 in order, but current stanc flips some of the t entries around

[WardBrian]

Closing in favor of #1742