stan-dev / stan-dev/math

stan::math and std::complex

Open
#3,006 8 comments 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
C++
Stars
839
Forks
220
Avg merge
2d 4h
Merged PRs (30d)
14

Description

Description

Currently, Stan's complex number support is entirely built on std::complex, including autodiff, which
uses std::complex<stan::math::var>.

This is, unfortunately, unspecified behavior in the C++ spec [26.4.2]:

The effect of instantiating the template complex for any type other than float, double, or long double is unspecified. The specializations complex<float>, complex<double>, and complex<long double> are literal types

For a reminder on what "unspecified behavior" means:

unspecified behavior - the behavior of the program varies between implementations, and the conforming implementation is not required to document the effects of each behavior. Each unspecified behavior results in one of a set of valid results.

Essentially, unspecified behavior is the same as "implementation-defined behavior" but without the requirement that implementations document what they are doing. This is also often taken to mean there are no backwards compatibility guarantees on any specific unspecified behavior.

This creates both a maintenance burden (each new libstdc++/libc++ release can create arbitrary amounts of work for our developers) and a stability hazard (the idea that "Stan X.Y will continue to work a year from now, without needing to update to Stan X.Z" is false as things stand today)

Problems

Recent versions of clang/libstdc++ have made changes which they are fully within their rights to do by the spec, but have broken Stan builds.

  • In libstdc++16, they changed the definition of log(complex) from complex<T>(log(abs(x)), arg(x)); to complex<T>(std::log(std::abs(x)), std::arg(x));. This broke argument dependent lookup for this function.
    A similar change broke operator* for our complex types.
    This lead to https://github.com/stan-dev/cmdstan/issues/1158, which was the reason we needed a 2.32.1 release.
    @andrjohns provided the fix in https://github.com/stan-dev/math/pull/2892
  • In libstdc++17, a similar change was made to fabs, which necessitated to https://github.com/stan-dev/math/pull/2991
  • In libstdc++19, the internal structure of pow was rewritten such that several overloads lead to a static assert failing if the type passed was not arithmetic: #3106

What to do

This is less clear to me.

Option 1 - walk on egg shells

So far, all of the issues that have arisen from this have been due to argument dependent lookup breaking for these types. We can fix that by being much more explicit, as we did in https://github.com/stan-dev/math/pull/2892 and https://github.com/stan-dev/math/pull/2991. This requires auditing the existing usages, which probably requires a fair amount of C++ expertise to understand how the calls are being resolved.

Option 2 - our own type

We could rather trivially define our own stan::math::complex<T> type. We could make it assignable from std::complex<double>, and I think be off to the races? I believe the complex linear algebra we use in Eigen all support a template argument for the complex type, rather than assuming std::complex.
This would require a fair amount of boilerplate to actually do any math on it, and in the case of double we may lose out on some of the optimizations that having the type built in to the language grants, but we'd own it.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by reviewing the current std::complex usage in Stan's autodiff and the fixes in math PRs #2892 and #2991, then compare the audit approach with introducing a stan::math::complex type. Examine the affected log, operator*, fabs, and pow paths and the Eigen complex-type assumptions. Done requires a decided approach that avoids dependence on unspecified std::complex behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
backend
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.