Broaden the template bound on the lp argument to constraints#3151
Broaden the template bound on the lp argument to constraints#3151
Conversation
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
| template <typename EigVec, require_eigen_vector_t<EigVec>* = nullptr> | ||
| template <typename EigVec, typename Lp, | ||
| require_eigen_vector_t<EigVec>* = nullptr, | ||
| require_convertible_t<return_type_t<EigVec>, Lp>* = nullptr> |
There was a problem hiding this comment.
So this will allow lp to be var and return_type_t<EigVec> to be double or var.
Do we ever want to increment the jacobian when y just has a scalar type of double?
lp += 0.5 * log1m(sum_sqs);What are the use types you want to pass into this? Is it just the return_type_t<EigVec> = double and lp = var case? Since the data is considered constant in that case shouldn't we just call the one that does not increment the jacobian?
There was a problem hiding this comment.
return_type_t<EigVec> = doubleandlp = var
Yes, that's the relevant one that comes up during code generation. Based on my conversation with @bob-carpenter, I think we still want to let the user do it, even if it's weird. The implementation in stanc3#1494 gives the user both a _jacobian function and a _constrain function, and I think it's reasonable to view these as similar to the difference between doing a target+= with a _lpdf versus a _lupdf.
| inline auto corr_matrix_constrain(const T& y, int K, Lp&... lp) { | ||
| static_assert(sizeof...(lp) == 0 || sizeof...(lp) == 1, | ||
| "corr_matrix_constrain should be called with either " | ||
| "two or three arguments"); | ||
| return apply_vector_unary<T>::apply( | ||
| y, [&lp..., K](auto&& v) { return corr_matrix_constrain(v, K, lp...); }); |
There was a problem hiding this comment.
This looks like a bug in our api since we only have the lp version for corr_matirix_constrain(std::vector<>). imo I'd rather just have two functions. It's more code but I think cleaner just to have two separate function
There was a problem hiding this comment.
You mean two separate overloads that both call apply_vector_unary? I can split them up if you want, it's just a bunch of copy-paste
There was a problem hiding this comment.
Thanks! Yes it's just a bunch of copy pastes but imo I'd rather have duplicate code that is a bit easier to read here
SteveBronder
left a comment
SteveBronder
left a comment
There was a problem hiding this comment.
So imo everything is good except that I would like to have a new signature in the places where we are doing LP.... That just looks kind of clunky and having two signatures just makes a bit more sense.
| * from transforming the specified finite vector of size K plus (K choose 2). | ||
| * This overload handles looping over the elements of a standard vector. | ||
| * | ||
| * @tparam Jacobian if `true`, increment log density accumulator with log |
There was a problem hiding this comment.
Add back docs for jacobian
There was a problem hiding this comment.
I think this is just github showing the diff weirdly because a signature moved around -- there is no jacobian template on this overload
|
@SteveBronder I pushed the change to split up the |
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
|
Did you want to also do |
|
It’s not used anywhere by the compiler, so I don’t currently plan on exposing it to the user |
|
@SteveBronder it looks like your latest push broke the accompanying stanc3 PR: https://jenkins.flatironinstitute.org/job/Stan/job/Stanc3/job/PR-1494/14/ |
WardBrian
force-pushed
the
broaden-constraint-lp-template
branch
from
e918b34 to
7866a45
Compare
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
3 participants
Summary
This makes the
lpargument of the_constrainfunctions its own template parameter rather that solely relying on the other arguments. This is useful for stan-dev/stanc3#1436, as for technical reasons it is hard to prevent the user from passing all doubles if these are exposed.It also changes the order of some overloads and uses some template pack tricks to allow the non-Lp versions of these functions to be vectorized as well.
Tests
Side Effects
Release notes
Checklist
Copyright holder: (fill in copyright holder information)
The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
- Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
- Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)
the basic tests are passing
./runTests.py test/unit)make test-headers)make test-math-dependencies)make doxygen)make cpplint)the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested