Issue 1232 - variadic is_nan function#1233
Issue 1232 - variadic is_nan function#1233syclik merged 21 commits intostan-dev:developfrom andrjohns:feature/issue-1232-variadic-is-nan
Conversation
syclik
left a comment
syclik
left a comment
There was a problem hiding this comment.
Requesting a quick doc change for clarity. Everything else looks awesome!
stan/math/prim/scal/fun/is_nan.hpp
Outdated
| } | ||
|
|
||
| /** | ||
| * Returns true if the input is NaN and false otherwise. |
There was a problem hiding this comment.
Can you make this doc more precise? It should indicate that it returns true if any of the input is NaN (as opposed to all).
|
I think what @bob-carpenter said is important: The function should become |
|
@syclik I've made the changes from the issue discussion and the tests have passed, this is ready for re-review |
syclik
left a comment
syclik
left a comment
There was a problem hiding this comment.
I left a few comments. Overall, this looks great! The major things we need to address:
is_any_nanhas all its arguments pass by value and it should really default to pass by reference. (Unless theTis actually being deduced asconst stan::math::var&when appropriate... if that's happening, just let me know.)- there's inconsistency with the prim version of
is_nanand the other function overloads. They should all be function overloads or shift to a templated function with template specializations.
| * Returns true if the input is NaN and false otherwise. | ||
| * | ||
| * Delegates to <code>stan::math::is_nan</code> so that | ||
| * appropriate specialisations can be loaded for autodiff |
There was a problem hiding this comment.
Minor: mind spelling “specialisations” as “specializations”? We’ve used American English throughout our docs and it’d be good to maintain consistency.
| * @return <code>true</code> if the value is NaN. | ||
| */ | ||
| template <typename T> | ||
| inline bool is_any_nan(T x) { |
There was a problem hiding this comment.
We should have caught this on the original PR that introduced is_nan. The arguments are passed by value here. They should be passed by reference for efficiency reasons. So... in the function signature: is_any_nan(T& x) instead of is_any_nan(T x).
| * @return <code>true</code> if any value is NaN | ||
| */ | ||
| template <typename T, typename... Ts> | ||
| inline bool is_any_nan(T x, Ts... xs) { |
There was a problem hiding this comment.
Same here. This should be by reference.
stan/math/prim/scal/fun/is_nan.hpp
Outdated
| */ | ||
| inline bool is_nan(double x) { return std::isnan(x); } | ||
| template <typename T> | ||
| inline bool is_nan(T x) { |
There was a problem hiding this comment.
Hmm... this looks odd. In develop, we have 3 definitions of is_nan: prim/scal, rev/scal, fwd/scal. They are implemented using function overloading.
I think you either want to go the same route or you want to make this function the template function (as you have it) and then change the implementations in rev and fwd to be template specializations.
There was a problem hiding this comment.
Does that make sense? If it's not clear what I'm talking about, we can discuss.
| inline typename return_type<T1, T2>::type log_falling_factorial(const T1 x, | ||
| const T2 n) { | ||
| if (is_nan(x) || is_nan(n)) | ||
| if (is_any_nan(x, n)) |
There was a problem hiding this comment.
[comment]
The changes look awesome! It does look like there's an impact on how we will write functions here on out.
|
@serban-nicusor-toptal would you be able to restart the "Always-run tests part 1" for this pull? Looks like the failure was just randomness, as discussed in #1238 |
|
@andrjohns Good morning! |
|
Thanks for that! |
|
@serban-nicusor-toptal looks like the build is running twice for some reason (there's a run 8 and a run 9), not sure what happened there |
|
I got the email and restarted it. Should have posted here sorry. |
|
No worries, I'm not about to complain for having too much help! |
|
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 1.03) |
|
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 1.0) |
|
@syclik This is ready for another look-over. I've updated I also added in fwd & rev tests for |
bob-carpenter
left a comment
bob-carpenter
left a comment
There was a problem hiding this comment.
So as not to overreview this and have you wait for my approval, I'm just going to click approve here. But there are some things that should be changed.
Thanks for taking all this on! Overall, these changes are big improvements. I'm just worrying about minor details here that will be important for consistency.
stan/math/fwd/scal/fun/is_nan.hpp
Outdated
| */ | ||
| template <typename T> | ||
| inline int is_nan(const fvar<T>& x) { | ||
| inline typename std::enable_if_t<is_fvar<T>::value, bool> is_nan(const T& x) { |
There was a problem hiding this comment.
What was the motivation for switching to enable_if_t from the simple pattern matching? I think this signature should just be:
template <typename T>
inline bool is_nan(const fvar<T>& x);
There was a problem hiding this comment.
This was one was based on @syclik's review:
Hmm... this looks odd. In develop, we have 3 definitions of is_nan: prim/scal, rev/scal, fwd/scal. They are implemented using function overloading.
I think you either want to go the same route or you want to make this function the template function (as you have it) and then change the implementations in rev and fwd to be template specializations.
| */ | ||
| template <typename T, typename... Ts> | ||
| inline bool is_any_nan(const T& x, const Ts&... xs) { | ||
| return (is_any_nan(x) || is_any_nan(std::forward<const Ts>(xs)...)); |
There was a problem hiding this comment.
These are both OK to be constant references so they'll be generic. You don't need those outer parens---that's an R thing, not a C++ thing and they're just a distraction here.
| */ | ||
| inline bool is_nan(double x) { return std::isnan(x); } | ||
| template <typename T> | ||
| inline typename std::enable_if_t<std::is_arithmetic<T>::value, bool> is_nan( |
There was a problem hiding this comment.
Same comment here as in the issue for changing all of these. At most, this should be
template <typename T>
inline typename std::enable_if_t<std::is_arithmetic<T>::value, bool> is_nan(T x) {
return std::isnan(x);
}
so primitives get passed by value.
…stable/2017-11-14)
|
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 1.0) |
|
@syclik This one is just waiting on your approval before it's ready to merge, thanks! |
syclik
left a comment
syclik
left a comment
There was a problem hiding this comment.
@andrjohns: most of it is good. I think there was confusion about how is_nan() should be implemented. I have the same question that @bob-carpenter had. Why do we need the enable_if_t at all?
In prim, it could be:
template <typename T>
inline bool is_nan(T x) {
return std::isnan(x);
}
in fwd:
template <>
template <typename T>
inline bool is_nan(const fvar<T>& x) {
return is_nan(x.val());
}
and rev:
template <>
inline bool is_nan(const var& x) {
return is_nan(x.val());
}
Is there something wrong with the approach above? In the Math library, where we can, we just use the template specializations directly rather than trying to do clever trait programming. Simpler is better for reading this code.
|
I've never been clear on where we should use template specializations and where we should just use overloads. Name resolution works by finding the best matching (template) function overload, then finding the best matching specialization. So I see how it works differently, just not when it would make a difference in a real situation.
… On May 26, 2019, at 9:38 PM, Daniel Lee ***@***.***> wrote:
@syclik commented on this pull request.
@andrjohns: most of it is good. I think there was confusion about how is_nan() should be implemented. I have the same question that @bob-carpenter had. Why do we need the enable_if_t at all?
In prim, it could be:
template <typename T>
inline bool is_nan(T x) {
return std::isnan(x);
}
in fwd:
template <>
template <typename T>
inline bool is_nan(const fvar<T>& x) {
return is_nan(x.val());
}
and rev:
template <>
inline bool is_nan(const var& x) {
return is_nan(x.val());
}
Is there something wrong with the approach above? In the Math library, where we can, we just use the template specializations directly rather than trying to do clever trait programming. Simpler is better for reading this code.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@bob-carpenter: I'm with you. I sort of view them as exchangeable for this sort of application (although someone might be able to tell me why one would be preferred). What I was trying to get at with my comments was that we don't need the |
|
@syclik @bob-carpenter |
|
Fine by me, but it's Daniel's call.
|
|
@syclik it's your call ^^^^^ (just trying to keep things moving. this is a good change). |
|
@andrjohns when you have the time, please merge the latest develop in. There are conflicts due to the meta flattening refactor. |
|
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 0.99) |
|
@syclik Did you have a preference on the specification of |
|
My preference is for simpler rather than more complex. Here, I'd prefer develop rather than using |
|
Great! Changes have been reverted |
|
@serban-nicusor-toptal If the windows threading errors aren't an issue anymore can you restart the testing for this one? Thanks |
|
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 1.0) |
8 participants
Summary
As initially described in #1232 - Several functions call
is_nanmultiple times using the||operator, such as inprim/scal/fun/grad_reg_inc_gamma:This PR adds a variadic extension to the
is_nanfunction so that multiple inputs can be passed in a single callTests
Added tests for calling
is_nanwith multiple inputsSide Effects
N/A
Checklist
Math issue Variadic is_nan function #1232
Copyright holder: Andrew Johnson
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 doxygen)make cpplint)the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested