math icon indicating copy to clipboard operation
math copied to clipboard

vectorize atan2

Open spinkney opened this issue 3 years ago • 9 comments

Summary

Vectorized atan2 and added a unit test.

Tests

There was no unit test for atan2 so I added for the scalar and vectorized cases.

Release notes

The function atan2 is vectorized on both the inputs.

Checklist

  • [x] Math issue #2809

  • [x] Copyright holder: Sean Pinkney

    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/)

  • [x] the basic tests are passing

    • unit tests pass (to run, use: ./runTests.py test/unit)
    • header checks pass, (make test-headers)
    • dependencies checks pass, (make test-math-dependencies)
    • docs build, (make doxygen)
    • code passes the built in C++ standards checks (make cpplint)
  • [x] the code is written in idiomatic C++ and changes are documented in the doxygen

  • [x] the new changes are tested

spinkney avatar Sep 09 '22 09:09 spinkney

@andrjohns 2 questions

  1. stan::test::expect_common_nonzero_binary(f); this errors out for atan2, do you know why?
  2. On the 2nd test in that beta file if I do it on atan2 I'm getting this error (as well as a bunch after) test/unit/math/mix/fun/atan2_test.cpp:42:12: error: no matching function for call to 'atan2' return atan2(x1, x2);

spinkney avatar Sep 13 '22 15:09 spinkney

So the 2nd error is caused by the expect_ad_matvar test, because the atan2 function doesn't have an overload for var_value<Eigen> types, like the beta function, so you can just remove those tests.

What was the expect_common_nonzero_binary error? I can't find it in the logs above sorry

andrjohns avatar Sep 14 '22 09:09 andrjohns

Yes, I just added the overload :). I thought it would just work without it, lol. Once I push we'll be able to see the expect_common_nonzero_binary errors.

spinkney avatar Sep 14 '22 09:09 spinkney

The first error that comes up is

./test/unit/math/expect_near_rel.hpp:77: Failure
Value of: stan::math::is_nan(x1) && stan::math::is_nan(x2)
  Actual: false
Expected: true
expect_near_rel(0, nan)
expect_near_rel; require items x1(0) = x2(0): gradient() grad for finite diff vs auto diff

./test/unit/math/expect_near_rel.hpp:77: Failure
Value of: stan::math::is_nan(x1) && stan::math::is_nan(x2)
  Actual: false
Expected: true
expect_near_rel(0, nan)
expect_near_rel; require items x1(0) = x2(0): gradient_fvar() grad

spinkney avatar Sep 14 '22 11:09 spinkney

I checked the values being used by the expect_common_nonzero_binary function, and it looks like the error occurs when the first argument to atan2 is infinite. You can reproduce with:

  stan::test::expect_ad(f, stan::math::INFTY, 1.0);

I would guess that there needs to be a check for finite values in the gradient calculations, otherwise one of the intermediate calculations results in a nan being returned

andrjohns avatar Sep 15 '22 08:09 andrjohns

That makes sense but I don't know how to achieve this. I tried a bunch of combinations of

  if (is_inf(a.val()) || is_nan(a.val()) || is_inf(b.val()) || is_nan(b.val())) {
     return make_callback_var(
      std::atan2(a.val(), b.val()), [a, b](const auto& vi) mutable {
        a.adj() += 0;
        b.adj() += 0;
      });
  }

which doesn't solve the issue. Plus, I'm not even sure that should work or is "idiomatic c++". I think I'll have to leave this unless you or someone else can fix the issue.

spinkney avatar Sep 15 '22 09:09 spinkney

@andrjohns I think I'll comment out expect_common_nonzero_binary and just test the derivative manually. Talking to @rok-cesnovar it seems that finite differences is failing for infinite input (as you noted too). However, I can't figure out how to just test expect_equal to nan for a derivative.

spinkney avatar Sep 15 '22 20:09 spinkney

@andrjohns I think I'll comment out expect_common_nonzero_binary and just test the derivative manually. Talking to @rok-cesnovar it seems that finite differences is failing for infinite input (as you noted too). However, I can't figure out how to just test expect_equal to nan for a derivative.

Sounds good to me! Ping me when this is ready for another look

andrjohns avatar Sep 16 '22 08:09 andrjohns

I can't figure out how to just test expect_equal to nan for a derivative.

@andrjohns, would you be able to help with this?

spinkney avatar Sep 23 '22 15:09 spinkney

This is good to go now, right?

rok-cesnovar avatar Oct 24 '22 07:10 rok-cesnovar