Skip to content

Add tuples to the language#1100

Merged
WardBrian merged 201 commits into
masterfrom
tuple-redo
Jul 21, 2023
Merged

Add tuples to the language#1100
WardBrian merged 201 commits into
masterfrom
tuple-redo

Conversation

@WardBrian
Copy link
Copy Markdown
Member

@WardBrian WardBrian commented Jan 26, 2022

This is a port of the changes @rybern made in #675 to master. I tried to first port this while preserving his comments, etc, and then made some changes myself to reflect the new status of things.

This will close stan-dev/stan#2431

Needs


Submission Checklist

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

Release notes

Add Tuples to the language

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)

Continue work from Ryan PR

Remainder of changes from Ryan's pr

Small fixes

Match existing test output
Projection is a bit more technical, but it mainly creates further distance from 'Indexed' for easier reading
@WardBrian WardBrian added the big-exciting-project Large projects that we're excited about but might take a long time and possibly not end up working o label Jan 26, 2022
@WardBrian
Copy link
Copy Markdown
Member Author

@SteveBronder I'm handing this off to you. If you run into any questions feel free to ping/email me as I've gotten quite familiar with the state of things.

@rok-cesnovar once we're in the freeze you may want to take a look at what is needed to support tuples in assign()

The first thing that needs to happen is work in the code gen for transformations/IO - currently we hit failwiths there that prevent code generation from proceeding. Once we get most of that sorted we can start iterating more on the rest of the PR. I am pretty sure the front end stuff is in a decent spot.

@WardBrian WardBrian marked this pull request as draft January 26, 2022 18:09
@WardBrian WardBrian linked an issue Feb 3, 2022 that may be closed by this pull request
@andrjohns
Copy link
Copy Markdown
Contributor

Any chance this implementation will support tuples of user-defined functions? Not a request if it's a lot of work or anything, just curious

@WardBrian
Copy link
Copy Markdown
Member Author

Do you mean as a return or argument type? Yes

@andrjohns
Copy link
Copy Markdown
Contributor

As an argument type, so in stan pseudo-code:

real fun_a(real a) {
  ...
}

real fun_b(real b) {
  ...
}

other_function(..., make_tuple(fun_a, fun_b));

I'm prototyping an approach for user-defined gradients, and it assumes that a tuple of arguments and a tuple of gradient functions will be passed:

real value_fun(real x, real y) {
  return x * y;
}
real dx_fun(real x, real y) {
  return y;
}
real dy_fun(real x, real y) {
  return x;
}



user_gradients(make_tuple(a, b), value_fun, make_tuple(dx_fun, dy_fun));

But if a tuple of functions/functors isn't feasible, then I can make it variadic and split the functions out in the c++

@WardBrian
Copy link
Copy Markdown
Member Author

I don’t think that would work completely out of the box, but it would be a minor change to generate a tuple of the functor structs in C++. I see no reason why such a thing wouldn’t be possible for a library function. For user defined functions, that signature would of course be impossible to define until we have a way of writing down a function’s type inside Stan

I have some more questions about your thinking on this but I’ll ask them on slack/not in this PR

@WardBrian
Copy link
Copy Markdown
Member Author

Is that something we'd desire being possible?

I think we would need to have the requirement that if any container is used in the constraint, rightmost index must be included (e.g., you could make lowers2 5,6,3 where the final dimension is just rep_array(value_you_want, 3)). So while the syntax you mentioned may be impossible, I think something semantically equivalent would be implementable

@WardBrian
Copy link
Copy Markdown
Member Author

The remaining uncertainties (marked TUPLE MAYBE) are all in the Memory Patterns optimizations, so I'd appreciate if @SteveBronder could take a closer look at those parts

@nhuurre
Copy link
Copy Markdown
Collaborator

nhuurre commented Jun 16, 2023

Is that something we'd desire being possible?

Probably not but since it wasn't in the design doc it's hard know for sure. If the current requirement were that if any container is used in the constraint, all indexes must be included, that would be consistent with either as a future extension and also currently allow something semantically equivalent.

@WardBrian
Copy link
Copy Markdown
Member Author

I'd be happy to open a PR revising the design doc if you think that is necessary for this PR to proceed. I think that the current implementation does not prevent us from doing (... something semantically equivalent to ..) what you've described in the future. The eventual situation would end up looking something like a two-part rule:

  1. A tuple can have entries which are constrained by the same rules the containing type. So a tuple(real<lower=0>, ...), or tuple(array[10] real<lower=1>, ...), or tuple(array[10] real<lower={...}>, ...)
  2. An array of tuples can have each array element have the same constraints, using the above syntax, or must be given a constraint which has dimensions that are the union of the "outer" array dimensions and any "inner" dimensions, so array[N] tuple(array[M] real<lower=X>, ...) is valid for X as a 1) scalar (currently allowed), 2 1-d array (currently allowed, must be of length M), 3) 2-d array ( not currently supported, but would require size N,M)

Regardless from whether the design would allow for it, I still have some reservations about actually allowing different tuples in the same array to have different internal constraints, but those can be saved for later discussions of that actual feature

@nhuurre
Copy link
Copy Markdown
Collaborator

nhuurre commented Jun 16, 2023

Yes, I think updating the design doc is the best way to move forward.

This argument has been a bit weird because I think we both agree what the eventual future is going to be. Specifically, it is one where all of the following declarations are accepted:

data {
  int N, M, K;
  real L;
  array[K] real Lk;
  array[M,K] real Lmk;
  array[N,M,K] real Lnmk;
}
parameters {
  array[N,M] tuple(array[K] real<lower=L>, real) T;
  array[N,M] tuple(array[K] real<lower=Lk>, real) Tk;
  array[N,M] tuple(array[K] real<lower=Lmk>, real) Tmk;
  array[N,M] tuple(array[K] real<lower=Lnmk>, real) Tnmk;
}

And we also agree that this is not something to implement right now but a plan for the far future, likely requiring a new design doc, or might never even happen.

Where we disagree is what the first concrete step toward that hypothetical future should be.

  • You think arrays should stay homogeneous and Tk is the only one consistent with that
  • I think we should steer clear of the potential ambiguity of Tk and instead implement Tnmk

Comment thread src/analysis_and_optimization/Mir_utils.ml Outdated
@WardBrian
Copy link
Copy Markdown
Member Author

Design doc changes stan-dev/design-docs#50

I think your summary is fair. I will admit that, besides thinking that it is the right thing to do, the fact that homogeneous elements is easiest to implement (even easier than if we only allowed scalars at first) does weigh in to breaking the tie between the different ideas

@WardBrian
Copy link
Copy Markdown
Member Author

Over this past weekend I did some fuzzing of this PR for the equivalent of 8 days (4 with —O1, 4 without). It found four crashes, 3 of which were the parsing issue fixed in d59e0e3 and the last was unrelated (#1336).

This isn’t the same as testing the generated C++ compiling obvious, but it does seem like we’ve resolved most/all of the internal exceptions which could have occurred (@nhuurre had already spotted a bunch manually before this)

Copy link
Copy Markdown
Contributor

@SteveBronder SteveBronder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C++ Code changes look good except for a few little spots.

Comment thread test/integration/good/compiler-optimizations/cppO1.expected Outdated
Comment thread test/integration/good/compiler-optimizations/cppO1.expected Outdated
@WardBrian WardBrian requested a review from SteveBronder July 20, 2023 15:05
Copy link
Copy Markdown
Contributor

@SteveBronder SteveBronder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ty! I looked over the rest of the code and read a lot of the C++ output and that looks good to me. @nhuurre unless you have any objections I think we are ready to merge!

Copy link
Copy Markdown
Collaborator

@nhuurre nhuurre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No objections.

@WardBrian WardBrian merged commit 054d554 into master Jul 21, 2023
@WardBrian WardBrian deleted the tuple-redo branch October 25, 2023 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

big-exciting-project Large projects that we're excited about but might take a long time and possibly not end up working o

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

How should tuples be represented in get_dims()? How should tuples be emitted by the --info option? add tuple type to Stan language

9 participants