Add deserializer class for reading constraints easier#3013
Conversation
SteveBronder
changed the title
…4.1 (tags/RELEASE_600/final)
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
…4.1 (tags/RELEASE_600/final)
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
|
@rok-cesnovar do you know what's up with the github check here? It's just giving back |
bbbales2
left a comment
bbbales2
left a comment
There was a problem hiding this comment.
Partial review to get comments on read-and-check-constraints versions of functions
| template <typename S, typename K> | ||
| using conditional_var_val_t | ||
| = std::conditional_t<is_var_matrix<S>::value && is_var<T>::value, | ||
| return_var_matrix_t<K, S, K>, K>; |
There was a problem hiding this comment.
Does this exist somewhere else in the code already? @rok-cesnovar do you recognize it?
There was a problem hiding this comment.
There was a problem hiding this comment.
@SteveBronder Oh yeah also this -- similar names but different function. We should rename this to just something kinda different so they don't get confused.
| require_not_vt_complex<Ret>* = nullptr> | ||
| auto read(Size m) { | ||
| if (unlikely(m == 0)) { | ||
| return map_vector_t(nullptr, m); |
There was a problem hiding this comment.
Is this the way the previous reader handed size zero things? This seems fine just curious.
There was a problem hiding this comment.
Yeah this is how it was done before.
Kind of a side note to this, @bob-carpenter is there a math reason we can't have size zero simplex vectors but can have size zero vectors?
There was a problem hiding this comment.
I think it's that simplex vectors have the sum to 1 constraint, and a length 0 thing doesn't sum to one.
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
yeah, this is unrelated. Happens on develop too. Will looki into it. |
bbbales2
left a comment
bbbales2
left a comment
There was a problem hiding this comment.
Here's the rest of the review on the code -- after the code gets sorted out I'll do the tests.
|
I think both. Docs will be good just to have them. The script is testing that the compiling works so it should be more thorough and automatic -- similar to expressions testing. |
|
Script is mostly written -- so hopefully won't be long turnaround time on this. There's also a temporary list over here: stan-dev/cmdstan#980 (comment) so that any compiler-writing can start based on that. |
|
Pinging @rybern FYI this should get merged tmrw so the compiler impl can start using it |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
|
@bbbales2 good to go? |
|
Can you push a commit here to bump jenkins? |
|
@SteveBronder yeah I'm happy with what is here. You good with it? Jump pushed a commit to get Jenkins to go again. |
…4.1 (tags/RELEASE_600/final)
|
Yep! |
|
Congrats!! Would a matching interface for the _free variables make sense? In stanc3, we generate the _constrain and _free calls in the same way, so this PR will let me simplify the _constrain calls, but with a new _free interface I can get rid of the old code altogether. (Same thing with checks to a lesser extent) |
|
Ty! Yes so So the inline void transform_inits(
const stan::io::var_context& context,
Eigen::Matrix<double, Eigen::Dynamic, 1>& params_r,
std::ostream* pstream = nullptr) const final {
//...
}But in Stan, Looking at both of those functions that use So I think we could just use a serializer and deserializer! What we could do is add a transform_inits(Eigen::VectorXd& input_r, Eigen::VectorXd& output_r, std::ostream* pstream__ = nullptr) {
stan::io::deserializer deserializer(input_r);
// Made stuff calling the read<>() functions.
Eigen::MatrixXd X = deserializer.read<Eigen::MatrixXd>(N, M);
// Write to serializer
stan::io::serializer serializer(output_r);
serializer.serialize_free_lb(X, lb);Thinking about it more could we just do the free's in deserializer and write transform_inits(Eigen::VectorXd& input_r, Eigen::VectorXd& output_r, std::ostream* pstream__ = nullptr) {
stan::io::deserializer deserializer(input_r);
stan::io::serializer serializer(output_r);
// Make stuff calling the read_free_lb<>() functions.
serializer.serialize(deserializer.read_free_lb<Eigen::MatrixXd>(lb, N, M));@bbbales2 do you have any opinions on whether the free functions should be in the serializer or deserializer? I think it would be nice to keep the serializer simple and just put all this in the serializer I have to double check that this works, but if so does that work for @rybern? My only concern is that we have to worry about parameter order, though last time I looked at the contexts it just takes the vector you give it and uses that data to create sub arrays of those dimensions internally so I think this would be fine. Tagging @betanalpha as I know he has had opinions on this in the past |
The upside here would be the cleanup right? That looks right to me.
Since we have the option, separate seems good. The things missing on this are like the array forms of I'm guessing that it would probably take 2-3 days to get the Math components of this through. Same with the Stan side of things. So if we started now I'd guess we'd be done by this time next week. If this cleans up the code quite a bit, then worth it. The only thing that worries me is:
I'm hoping that "add" is actually a "replace"? Cause I assume we'd be deleting the old-style transform_inits. Touching the var_contexts scares me a bit cause I don't know what downstream code exists that would need to be updated. At worst we could just serialize to a local variable and then push those values onto the var_context. This part of the code shouldn't be performance sensitive. |
Yeah we could either do these in the deserializer (since it's just calling
Yeah so I was thinking of a triage'd style thing where we
Unless we want to do one big PR in Stan that does (1) and (3), then merge them at the same time as Ryan updates the stanc3 code
Yeah I agree, like looking around it seems like this is fine but we should probs reach out to folks we wrote this stuff to confirm what we are talking about here is going to work smoothly |
|
Hmm well yeah since you mention it all the array free stuff can just go here in a Since you have a picture of serializer in your head, you want to go ahead and kick that off? I remain inclined to ditch all things integer here, fwiw. |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
|
Yeah sure I'm trying to put together some docs for the compiler today and I'm going to give up on that in like 20 mins or so and I'll ge started on this |
|
It won't let me click the merge button but Ben feel free to merge. @rok-cesnovar do I need some privileges I don't have? Tagging @syclik as well |
|
You have write privileges in this repo so this is definitely weird. |
|
Sorry for the delay! @SteveBronder I think what you described works great for my purposes (I probably haven't grokked the whole story though)
I do think it'll simplify of the hairier bits of OCaml, because the logic would be easier and match the new constraints, and it'll generalize better for tuples. Also, maybe we won't have to deal with _free__ variables across unrelated parts of the compiler. It might have a little performance gain too, right? Just to make things concrete for my benefit, here's @nhuurre's example: For constraints we can now generate: For I'm imagining that it's possible to do something more direct along the lines of |
|
This has the code to do something like: I think Steve's working on a replacement for the |
|
Awesome! |
|
Yeah Ryan you may actually be able to write something like stan::io::deserializer deserializer(input_vec__)
stan::io::serializer serializer(vars__);
serialize.write(deserializer.free_lb<std::vector<std::vector<Matrix>>>(lb, lp, {dims...})Is it just those free functions called in transform inits or does something else happen? |
|
@SteveBronder sorry Steve I missed that you asked a question here
I think in transform_inits it's just doing the free stuff, @seantalts could you confirm? |
6 participants
Submission Checklist
./runTests.py src/test/unitmake cpplintSummary
This does the FR for #3005 by making a
deserializerclass that handles constraints for the types and bounds used in the Stan compiler. It's a bit of a monster but handles pretty much all the parameter sizes we have in Stan. The idea here is that instead of the compiler generating code for paremeters likelocal_scalar_t__ sd_1 = in__.scalar(); current_statement__ = 5; if (jacobian__) { current_statement__ = 5; sd_1 = stan::math::lb_constrain(sd_1, 0, lp__); } else { current_statement__ = 5; sd_1 = stan::math::lb_constrain(sd_1, 0); }It can instead just use
local_scalar_t__ sd_1 = in__.read_lb<local_scalar_t__, Jacobian__>(lp__, 0);This works for eigen matrices and arrays containing arrays so something like the following is perfectly legal
For general reading the function is
read<RETURN_TYPE>(dims)whereRETURN_TYPEis the type requested back and dims are the dimensions of the type. All of the constrain functions have the formread_{constrain}<{return_type}, Jacobian>({constrain_args}, lp, {dims})Where
{constrain}is the kind of the constraint likelb,ub,lubetc,{return_type}is the requested return type,Jacobianis whether to account for the Jacobian calculationJacobianis trueThis also adds
read()methods for complex scalars, vectors, and matrices.Intended Effect
This should simplify some of the compiler implementation for things like the new matrix type while also making things more performant by utilizing vectorized versions of the constrain functions when they are available.
How to Verify
About 2/3rds of this PR are testing, where we run all the standard tests from
reader, then also checkstd::vectorreturn types andvar_value<Eigen>returns for all matrix tests. You can run the tests withSide Effects
There's two things I still need to add here, tests for the complex stuff and tests for matrices with the
offset_and_multiplierconstraints. I may need to make a Stan math PR for offset multiplierDocumentation
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): Steve Bronder
By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses: