Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

min:max indexing does not support SoA but does not prevent this at compile time? #3244

Closed
WardBrian opened this issue Nov 27, 2023 · 5 comments · Fixed by stan-dev/math#2978
Assignees
Labels

Comments

@WardBrian
Copy link
Member

Summary:

Originally reported by @avehtari using a larger model. This is breaks sampling behavior when using STANCFLAGS=--O1

Reproducible Steps:

transformed data {
  int N = 10;
  int<upper=N> M = 10;
}
parameters {
  vector[N] x;
}
transformed parameters {
  vector[M] y;
  y = x[1 : M];
}

model {
  y ~ normal(0, 1);
}

When compiled with cmdstan and sampled, this does perfectly well. When you re-compile with STANCFLAGS=--O1, which flips on SoA for x and y, the sampling behavior becomes incredibly slow and the resulting ESS is horrible.

Current Output:

See above

Expected Output:

Either this should work, or if it is not intended to be used with SoA I would expect C++ compilation to fail. There are no uses of index_min_max in https://github.com/stan-dev/stan/blob/develop/src/stan/model/indexing/rvalue_varmat.hpp and no guards against it in https://github.com/stan-dev/stan/blob/develop/src/stan/model/indexing/rvalue.hpp, so those are the signatures being used

Additional Information:

Current Version:

v2.33.0

@bob-carpenter
Copy link
Contributor

Indexing into an SOA matrix should be efficient, so it shouldn't be too expensive to create the non-SOA matrix x[1:M]. Then we can assign it to the SOA matrix y, we just need the chain() on the SOA data type to add all the adjoints of y to the adjoints of x. Not super efficient, but it shouldn't be dreadful.

@SteveBronder
Copy link
Collaborator

SteveBronder commented Nov 27, 2023

That's odd, it should work. Let me look at this today it sounds like a bug in the rvalue function for min max indexing

@WardBrian
Copy link
Member Author

I just opened another issue on Math, but worth noting here that replacing the indexing with a call to head or segment also leads to failure (though in that case, it is a segfault rather than junk inference)

@SteveBronder
Copy link
Collaborator

Okay yeah digging into this it's an issue in the math library. I'm going to put up a PR in a minute

@SteveBronder
Copy link
Collaborator

Closing as this was a bug in math

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants