-
-
Notifications
You must be signed in to change notification settings - Fork 189
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
Add sum_to_zero constraint, free, and check based on ILR transform #3101
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: |
the math and tests look good to me! @SteveBronder I'll let you comment on the cpp style |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few small things but overall good!
|
||
typename plain_type_t<Vec>::Scalar sum_w(0); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typename plain_type_t<Vec>::Scalar sum_w(0); | |
value_type_t<Vec> sum_w(0); |
if (unlikely(y.size() == 0)) { | ||
return arena_t<ret_type>(Eigen::VectorXd{{0}}); | ||
} | ||
auto arena_y = to_arena(y); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
auto arena_y = to_arena(y); | |
auto arena_y = to_arena(std::forward<T>(y)); |
* @return Zero-sum vector of dimensionality K. | ||
*/ | ||
template <typename T, require_rev_col_vector_t<T>* = nullptr> | ||
inline auto sum_to_zero_constrain(const T& y) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inline auto sum_to_zero_constrain(const T& y) { | |
inline auto sum_to_zero_constrain(T&& y) { |
inline auto sum_to_zero_constrain(const T& y, scalar_type_t<T>& lp) { | ||
return sum_to_zero_constrain(y); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inline auto sum_to_zero_constrain(const T& y, scalar_type_t<T>& lp) { | |
return sum_to_zero_constrain(y); | |
inline auto sum_to_zero_constrain(T&& y, scalar_type_t<T>& lp) { | |
return sum_to_zero_constrain(std::forward<T>(y)); |
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: |
Summary
This is an alternative to #3099 based on the discussion in the comments there. Rather than
x[1:N-1] = y; x[N] = -sum(y)
this uses a modification of the isometric log ratio (ILR) transformation for simplexes. The implementation is based on a single for-loop, but was adapted from Sean's code that used several temporary vectors, which was adapted from a version which used a full matrixTests
Added tests in the err and constraint folders, based on the existing tests for
simplex.
In addition, I iterated on the implementation of this algorithm in Python, so I have some out-of-tree tests here: https://gist.github.com/WardBrian/aae35c65fb841f923d89cc30a3d70808
Side Effects
Release notes
Added
sum_to_zero_constrain
,sum_to_zero_free
, andcheck_sum_to_zero
.Checklist
Copyright holder: Simons Foundation
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