feature/elementwise_check (2) #1798
Conversation
Implement is_nonnegative and is_positive_finite. Fix error behavior for csr_u_to_z with out of range indices. Fix check_positive at 0. 0 is not positive.
…xp1~20180509124008.99 (branches/release_50)
| struct finite<Eigen::Matrix<T, R, C>, true> { | ||
| static void check(const char* function, const char* name, | ||
| const Eigen::Matrix<T, R, C>& y) { | ||
| if (!value_of(y).allFinite()) { |
There was a problem hiding this comment.
We want this allFinite call I think. (It does need to be value_of_rec(y).) This gives Eigen a chance to do something clever. See also: #1005. I'm not really sure what to do here?
There was a problem hiding this comment.
@peterwicksstringfield, are you suggesting this should be specialized for Eigen types? That would make sense to me. Especially if we could have a microbenchmark showing the difference in performance.
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
|
@peterwicksstringfield, this is awesome! I think you brought up a comment about performance. It seems like it's something we need to check before we consider this change, if I understood your comment correctly. Otherwise, this change cleans up a lot of code; thank you! |
bbbales2
left a comment
bbbales2
left a comment
There was a problem hiding this comment.
@peterwicksstringfield I needed something like this for a pull request I was working on (#1962), so I went through and reviewed this and I agree with @syclik that this is good to go.
I can write a couple microbenchmarks on the checks if you think they're necessary.
I have one question about the csr stuff, and the release notes can go in the pull request text instead of into the release notes file itself.
Do you have time to look at this?
|
Yo @SteveBronder, do you know the answer to the question here: #1798 (comment)? I see you have a pull request open with something about |
|
@peterwicksstringfield I pulled out the release notes and put them in the pull request and merged develop into your branch. @syclik can you give my changes a once over? I guess I'm not supposed to merge this now that I've touched it. |
…m/peterwicksstringfield/math into feature/elementwise_checks_part_2
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
|
@serban-nicusor-toptal Did the Windows test computer go down: https://jenkins.mc-stan.org/blue/organizations/jenkins/Stan/detail/downstream_tests/1745/pipeline ? |
|
Hey @bbbales2, one of the windows machines was unstable. I've turned it off for the moment and spin up a temporary one to not block the builds. |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
|
@bbbales2, I'm assuming the release notes are correct. The changes to the test look fine. |
8 participants
Summary
Allow check_not_nan and friends to work on nested containers. The next PR will eliminate a bunch of boilerplate loops.
These functions now operate on nested containers:
Adds new functions is_nonnegative and is_positive_finite. This is part of #50, but it fits in with this PR really well.
Fix a bug where check_positive("someFunction", "x", 0u) would not throw a domain_error.
Improve the error messages for csr_u_to_z when called with out of range indices.
Release Notes
Features:
New functions is_nonnegative and is_positive_finite to parallel check_nonnegative and check_positive_finite. They signal failure by returning false instead of by throwing std::domain_error.
Functions check_not_nan, check_nonnegative, check_positive, check_finite, check_positive_finite, is_not_nan, is_nonnegative, is_positive, is_scal_finite, and is_positive_finite now operate on nested containers.
Clearer error messages when csr_u_to_z is called with out of range indices.
check_positive now throws domain error when given an unsigned 0.
Tests
Tests for (check|is)_(nonnegative|positive) at 0.
Tests for the new functions is_nonnegative and is_positive_finite.
Tests to ensure that the vectorization is working.
Update some hardcoded error messages in other tests.
Fix the tests for csr_u_to_z.
Side Effects
Changes the format of some error messages.
Old:
New:
Where y is a matrix that contains a nan.
Release notes
New functions is_nonnegative and is_positive_finite to parallel check_nonnegative and check_positive_finite. They signal failure by returning false instead of by throwing std::domain_error.
Functions check_not_nan, check_nonnegative, check_positive, check_finite, check_positive_finite, is_not_nan, is_nonnegative, is_positive, is_scal_finite, and is_positive_finite now operate on nested containers.
Clearer error messages when csr_u_to_z is called with out of range indices.
check_positive now throws domain error when given an unsigned 0.
Checklist
Math issue Deeply nested containers and error checks #1635 (don't close)
Copyright holder: Peter Wicks Stringfield
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
the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested