Skip to content

allows complex abs() to be vectorized #2737

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

Merged
merged 3 commits into from
May 14, 2022

Conversation

SteveBronder
Copy link
Collaborator

Summary

Alternative to #2734 that uses apply_scalar_unary to vectorize abs() for complex types and containers of complex types. Mostly we just remove the explicit instantiations for std::vector<> and Eigen::Matrix types and use the apply_scalar_unary scheme to do the vectorization.

Tests

@bob-carpenter I kept the tests you added in the above PR and added a few more to make sure everything still works with the new matrix type as well.

Side Effects

idt so, but adding as a note @andrjohns we should add a doc page describing how to write vectorized versions of scalar functions.

Release notes

  • Allows complex abs() to be vectorized

Checklist

  • Math issue fully vectorize abs for int, real, complex #2729

  • Copyright holder: Steve Bronder

    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

    • 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)
  • the code is written in idiomatic C++ and changes are documented in the doxygen

  • the new changes are tested

@bob-carpenter
Copy link
Member

Thanks so much @SteveBronder. I think given that I've worked on this and don't clearly understand the current math requirements, that @rok-cesnovar or someone else is going to have to review this.

Copy link
Member

@rok-cesnovar rok-cesnovar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks.

@rok-cesnovar rok-cesnovar merged commit a0511be into develop May 14, 2022
@rok-cesnovar rok-cesnovar deleted the feature/complex-abs-vectorized branch May 14, 2022 12:25
@rok-cesnovar rok-cesnovar mentioned this pull request May 14, 2022
5 tasks
@WardBrian
Copy link
Member

Did this also vectorize the integer signature of abs?

@rok-cesnovar
Copy link
Member

rok-cesnovar commented May 14, 2022

Yes.

std::vector<int> t{1,2,-3,4,-5,6,-1};
EXPECT_EQ(stan::math::abs(t), std::vector<int>({1, 2, 3, 4,5,6, 1}));

passes.

@WardBrian
Copy link
Member

The relevant stanc pr is failing to compile the example model still:
https://jenkins.flatironinstitute.org/blue/rest/organizations/jenkins/pipelines/Stan/pipelines/Stanc3/branches/PR-1195/runs/8/nodes/298/steps/416/log/?start=0

/home/jenkins/workspace/Stan_Stanc3_PR-1195/performance-tests-cmdstan/cmdstan/stan/src/stan/model/indexing/assign.hpp:782:7: note: in instantiation of function template specialization 'stan::model::assign<std::vector<double, std::allocator<double> > &, std::vector<std::complex<double>, std::allocator<std::complex<double> > >, nullptr, nullptr>' requested here
../../test/integration/good/function-signatures/math/matrix/abs.hpp:1563:20: note: in instantiation of function template specialization 'stan::model::assign<std::vector<std::vector<double, std::allocator<double> >, std::allocator<std::vector<double, std::allocator<double> > > > &, std::vector<std::vector<std::complex<double>, std::allocator<std::complex<double> > >, std::allocator<std::vector<std::complex<double>, std::allocator<std::complex<double> > > > >, nullptr, nullptr>' requested here
      stan::model::assign(trans_x2z, stan::math::abs(cx2z),
                   ^

It seems like the return type of the vectorized version may be incorrect?

@WardBrian
Copy link
Member

I can confirm this PR is broken for vectors of complex values.

Adding this test exposes the issue:

  std::vector<std::complex<stan::math::var>> vc {3};
  std::vector<stan::math::var> vr = abs(vc);

Failure:

------------------------------------------------------------
make -j1 test/unit/math/mix/fun/abs_test
g++ -DSTAN_TEST_PRINT_MATRIX_FAILURE -std=c++1y -pthread -D_REENTRANT -Wno-sign-compare -Wno-ignored-attributes      -I lib/tbb_2020.3/include    -O0  -I . -I lib/eigen_3.3.9 -I lib/boost_1.78.0 -I lib/sundials_6.1.1/include -I lib/sundials_6.1.1/src/sundials -I lib/benchmark_1.5.1/googletest/googletest/include -I lib/benchmark_1.5.1/googletest/googletest     -DBOOST_DISABLE_ASSERTS         -DGTEST_HAS_PTHREAD=0  -c -MT test/unit/math/mix/fun/abs_test.o -MM -E -MG -MP -MF test/unit/math/mix/fun/abs_test.d test/unit/math/mix/fun/abs_test.cpp
g++ -DSTAN_TEST_PRINT_MATRIX_FAILURE -std=c++1y -pthread -D_REENTRANT -Wno-sign-compare -Wno-ignored-attributes      -I lib/tbb_2020.3/include    -O0  -I . -I lib/eigen_3.3.9 -I lib/boost_1.78.0 -I lib/sundials_6.1.1/include -I lib/sundials_6.1.1/src/sundials -I lib/benchmark_1.5.1/googletest/googletest/include -I lib/benchmark_1.5.1/googletest/googletest -I lib/benchmark_1.5.1/googletest/googletest/include -I lib/benchmark_1.5.1/googletest/googletest      -DBOOST_DISABLE_ASSERTS         -DGTEST_HAS_PTHREAD=0 -DGTEST_HAS_PTHREAD=0  -c -o test/unit/math/mix/fun/abs_test.o test/unit/math/mix/fun/abs_test.cpp
test/unit/math/mix/fun/abs_test.cpp: In member function ‘virtual void mixFun_absReturnType_Test::TestBody()’:
test/unit/math/mix/fun/abs_test.cpp:228:40: error: conversion from ‘vector<std::complex<stan::math::var_value<double> >,allocator<std::complex<stan::math::var_value<double> >>>’ to non-scalar type ‘vector<stan::math::var_value<double>,allocator<stan::math::var_value<double>>>’ requested
  228 |   std::vector<stan::math::var> vr = abs(vc);
      |                                     ~~~^~~~
make: *** [<builtin>: test/unit/math/mix/fun/abs_test.o] Error 1
make -j1 test/unit/math/mix/fun/abs_test failed
exit now (05/16/22 09:53:37 EDT)

@WardBrian
Copy link
Member

  Eigen::MatrixXcd mc(0, 0);
  Eigen::MatrixXd mr = abs(mc);

Also fails with a similar error ("cannot convert 'const std::complex<double>' to 'double' in assignment")

@WardBrian
Copy link
Member

Here's a version of the test which I think should pass at a minimum. The first two sections are already tested and pass, and the integer section also passes. The middle versions which use containers of complex values fail to compile.

TEST(mixFun, absReturnType) {
  using stan::math::abs;

  // validate return types not overpromoted to complex by assignability
  std::complex<stan::math::var> a = 3;
  stan::math::var b = abs(a);

  std::complex<stan::math::fvar<double>> c = 3;
  stan::math::fvar<double> d = abs(c);

  std::vector<std::complex<stan::math::var>> vc{3};
  std::vector<stan::math::var> vr = abs(vc);

  Eigen::MatrixXcd mc(0, 0);
  Eigen::MatrixXd mr = abs(mc);

  std::vector<Eigen::MatrixXcd> vmc{mc};
  std::vector<Eigen::MatrixXd> vmr = abs(vmc);

  std::vector<int> t{1, 2, -3, 4, -5, 6, -1};
  // does not promote to real
  t = abs(t);

  SUCCEED();
}

It wouldn't hurt to add tests which also check that the correct values are returned for abs(complex) and its containers - that probably also would have caught this

@rok-cesnovar
Copy link
Member

Will look into this tomorrow. Thanks for digging all this up!

@SteveBronder
Copy link
Collaborator Author

I have a fix for this sorted out I'll put up in a little bit

@bob-carpenter
Copy link
Member

You can write tests using std::is_same<> for checking return types:

#include <typetraits>

template <typename E, typename T>
bool is_type(const T& x) {
  return std::is_same<T, E>::value;
}

and then call it with something like is_type<int>(abs(1)) to check. I think I wrote that into the primitive tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants