Added prototype free functions to deserializer#3018
Conversation
bbbales2
changed the title
…4.1 (tags/RELEASE_600/final)
…-dev/stan into feature/free-functions-deserializer
…4.1 (tags/RELEASE_600/final)
bbbales2
changed the title
…4.1 (tags/RELEASE_600/final)
|
@SteveBronder I think these should be copy-pasted into the serializer class. They aren't making use of anything in deserializer. Otherwise I think all the frees are here and the new bits are tested. |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
SteveBronder
left a comment
SteveBronder
left a comment
There was a problem hiding this comment.
Looks good! Quick q for @rybern on the API pattern here and we should be good to go.
| * @return Unconstrained variable | ||
| */ | ||
| template <typename S, typename L, typename U> | ||
| inline auto free_lub(const S& y, const L& lb, const U& ub) { |
There was a problem hiding this comment.
[optional] @rybern is it okay that the name pattern here is
free_{constrain_name}(object, {constrain_params});
Or would it be easier for you if we had the params come first like in the constrain functions?
Also @bbbales2 should we name these read_free_{constrain_name} and change the constrain functions to read_constrain_{constrain_name}? That would just help keep the API more consistent
There was a problem hiding this comment.
I don't think that'll make it any easier or harder for me, up to you.
Edit: it would actually make it easier for me if lp__ came before the constraint parameters in the read constraint functions, but I should have mentioned that before and I can work around it.
This looks awesome!
| std::vector<std::vector<Eigen::VectorXd>> std_std_vec | ||
| = {{vec1, vec2}, {vec2, vec1}}; |
There was a problem hiding this comment.
Would have saved us a lot of code if I had remembered that std::vector versions just call the underlying and could have done them all in one go. Very nice!
SteveBronder
left a comment
SteveBronder
left a comment
There was a problem hiding this comment.
Actually Ben I goofed here approving this. The signature should match the read functions like
template <typename Ret, require_not_std_vector_t<Ret>* = nullptr>
inline auto free_ordered(dims...) {Our goal is to replace the code here
https://gist.github.com/SteveBronder/283a8cc24787383232d26590e207ba0c#file-original_model-hpp-L1076
double x_param_scal;
x_param_scal = std::numeric_limits<double>::quiet_NaN();
x_param_scal = context__.vals_r("x_param_scal")[(1 - 1)];
double x_param_scal_free__;
x_param_scal_free__ = std::numeric_limits<double>::quiet_NaN();
x_param_scal_free__ = stan::math::lb_free(x_param_scal, 0);With code like
double x_param_scal_free__ = deserializer.read_free_lb<double>(0);
…/free-functions-deserializer
…4.1 (tags/RELEASE_600/final)
|
@SteveBronder it was way easier to write tests with the serializer in place, so this will wait on that pull to go in. There are a couple more changes I still need to add but will get them tomorrow or something:
|
|
Aight the serializer pr should be good to go |
…/free-functions-deserializer
bbbales2
changed the title
|
@SteveBronder This should be good to go once #3019 is in |
…4.1 (tags/RELEASE_600/final)
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
…ions-deserializer
|
@serban-nicusor-toptal Looks like Jenkins is throwing out of space errors: |
…ions-deserializer
…-dev/stan into feature/free-functions-deserializer
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
|
@rybern fyi we are going with the signatures |
|
@SteveBronder this is green |
|
Let me give it one quick read through though should be good |
|
@SteveBronder excellent |
| template <typename Ret> | ||
| inline auto read_free_simplex(size_t size) { | ||
| return stan::math::simplex_free(this->read<Ret>(size)); | ||
| } | ||
|
|
||
| /** | ||
| * Read serialized simplices and unconstrain them | ||
| * | ||
| * @tparam Ret Type of output | ||
| * @tparam Sizes Types of dimensions of output | ||
| * @param vecsize Vector size | ||
| * @param sizes dimensions | ||
| * @return Unconstrained vectors | ||
| */ | ||
| template <typename Ret, typename... Sizes, | ||
| require_std_vector_t<Ret>* = nullptr> | ||
| inline auto read_free_simplex(size_t vecsize, Sizes... sizes) { | ||
| std::decay_t<Ret> ret; | ||
| ret.reserve(vecsize); | ||
| for (size_t i = 0; i < vecsize; ++i) { | ||
| ret.emplace_back(read_free_simplex<value_type_t<Ret>>(sizes...)); | ||
| } | ||
| return ret; | ||
| } |
There was a problem hiding this comment.
Does this need a
template <typename Ret, require_not_std_vector_t<Ret>* = nullptr>
inline auto read_free_simplex(size_t size) {Or nah because you can't have a std::vector simplex? I'm pretty sure now we don't need it
There was a problem hiding this comment.
I added it anyway for symmetry with everything else lol. Pretty sure we don't need it too cause the number of size arguments will define which thing gets used.
|
It won't let me click the button for some reason but this is good to go!
|
|
My merge button is here. I'll click it if @rok-cesnovar hasn't clicked it by the AM. |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
6 participants
Submission Checklist
./runTests.py src/test/unitmake cpplintSummary
I added an example of how unit vector free would work
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): Columbia University
By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses: