Allow infinite log odds in categorical_logit and multinomial_logit - #3412
godking123 wants to merge 8 commits into
Conversation
…tests and case comments
|
Hi, @godking123, and thanks for the PR. Given that this is in our C++ code, we're going to need someone like @SteveBronder to review it. I'll leave some comments, but don't feel confident enough about our C++ style and library to do the final review. |
bob-carpenter
left a comment
bob-carpenter
left a comment
There was a problem hiding this comment.
I was confused in reading the code about why the checks were laid out how they were. The documentation also needs to change for the top-level function to indicate what does and doesn't throw.
Otherwise, I'm going to leave this to @SteveBronder.
| // Autodiff args must be finite, data can be +/-inf | ||
| if constexpr (is_constant<T_prob>::value) { | ||
| check_not_nan(function, "log odds parameter", beta_ref); | ||
| check_greater(function, "log odds parameter", beta_ref.maxCoeff(), |
There was a problem hiding this comment.
Why is this allowing +infinity, but not -infinity? I thought the issue was to have -infinity turn into zero.
There was a problem hiding this comment.
This check is for the all -inf case, it throws when the maximum in the beta vector is -inf. I realize this test isn't very straightforward I can reword it or add a comment
| check_greater(function, "log odds parameter", beta_ref.maxCoeff(), | ||
| NEGATIVE_INFTY); | ||
| } else { | ||
| check_finite(function, "log odds parameter", beta_ref); |
There was a problem hiding this comment.
Why are we checking the log odds parameter is finite? Isn't this allowed to be infinite?
There was a problem hiding this comment.
This is in the autodiff case, when it's a data variable it's allowed to be infinite
| if constexpr (is_constant<T_prob>::value) { | ||
| int num_infty = (beta_ref.array() == INFTY).count(); | ||
| if (num_infty > 0) { | ||
| return beta_ref.coeff(n - 1) == INFTY ? -std::log(num_infty) |
There was a problem hiding this comment.
Why are we taking the negative log of the number of infinite coefficients?
There was a problem hiding this comment.
We need log(1/num_infty) for log probability since the probability is 1/num_infty. -log(num_infty) is an equivalent expression to this
| ref_type_t<T_prob> beta_ref = beta; | ||
| check_finite(function, "log odds parameter", beta_ref); | ||
|
|
||
| // Autodiff args must be finite, data can be +/-inf |
There was a problem hiding this comment.
Thanks---this comment is useful---maybe something like this above to explain the range checks?
| // Autodiff args must be finite, data can be +/-inf | ||
| if constexpr (is_constant<T_prob>::value) { | ||
| check_not_nan(function, "log odds parameter", beta_ref); | ||
| if (beta_ref.size() > 0) { |
There was a problem hiding this comment.
Is beta_ref.size() == 0 legal at this point or was it short-circuited earlier?
There was a problem hiding this comment.
Yes should be legal it's reachable by an empty beta vector
|
Hi @bob-carpenter, thanks for the review. I've added comments clarifying the bounds checks and and updated the docstrings to spell out what throws, could you restart the CI checks for the latest commit whenever you get the chance? |
This branch has not been deployed
3 participants
Closes #3201.
Summary
categorical_logitandmultinomial_logitrejected any infinite value inbeta, althoughsoftmaxhandles-inf, socategorical_logit(beta)was stricter thancategorical(softmax(beta)). For databeta, the_rngand_lpmffunctions (and so_lupmf) now accept infinite values, as discussed in the issue:-infentries have zero probability.+infentries split the probability evenly between them. A single+infentry is deterministic.-infinput and NaN throw astd::domain_error.Autodiff
betain the_lpmffunctions must still be finite, since infinite values would break the gradients.When there are
+infentries, the probabilities are computed directly, becausesoftmax/log_sum_expwould give NaN. Otherwise the existing code path is used unchanged.categorical_logit_rng's sampling loop now uses>=instead of>, so a uniform draw of exactly0.0can't land on a zero-probability first entry.Tests
+/-infand to throw on all--inf.+infentries, for both_rngand_lpmf.betawith-infstill throws.Side Effects
None
Release notes
categorical_logitandmultinomial_logitnow accept infinite log odds whenbetais data:
-infentries get zero probability, and +inf entries split the probability evenly among themselves.Checklist
Copyright holder: Rajit Sareen
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