Feature/0123 complex spec #1720
Conversation
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
|
@serban-nicusor-toptal There was a failure that looked like it was due to email: https://jenkins.mc-stan.org/blue/organizations/jenkins/Math%20Pipeline/detail/PR-1720/1/pipeline/173 Here's the text from the failure from Always-run tests part 2, Windows Headers & Unit. |
|
Hey Bob, I can't be sure now if it's a Jenkins issue, I see that it's now running. Please let me know if it fails again with the same reason and I'll look into it. Thanks |
|
@serban-nicusor-toptal --- yes, it failed again with the same error. I think that may be because the windows machine was offline. @bgoodri --- do you know if it's online again? |
|
Back online. Had to pull its power cord out of the socket.
…On Mon, Feb 17, 2020 at 1:21 PM Bob Carpenter ***@***.***> wrote:
@serban-nicusor-toptal <https://github.com/serban-nicusor-toptal> ---
yes, it failed again with the same error. I think that may be because the
windows machine was offline. @bgoodri <https://github.com/bgoodri> --- do
you know if it's online again?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1720?email_source=notifications&email_token=AAZ2XKRPPRPDPGFNPUXTOETRDLIRBA5CNFSM4KWHI7P2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEL7JUOA#issuecomment-587110968>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZ2XKU2NPP7X3RE556TGATRDLIRBANCNFSM4KWHI7PQ>
.
|
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
Thanks, @bgoodri. Now all I need is a review. |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
stan/math/fwd/core/std_complex.hpp
Outdated
| /** | ||
| * Destroy this complex number. | ||
| */ | ||
| ~complex() {} |
There was a problem hiding this comment.
Are you able to use the implicit destructor?
There was a problem hiding this comment.
I was trying to follow the "rule of three". From the Wikipedia:
The rule of three (also known as the Law of The Big Three or The Big Three) is a rule of thumb in C++ (prior to C++11) that claims that if a class defines any of the following then it should probably explicitly define all three: destructor. copy constructor. copy assignment operator.
I'm overloading the define of the copy operator to deal with non-matching types (like assigning std::complex<double> to std::complex<var>. So I'm not sure where this falls. I'm happy to get rid of it if it's OK style.
There was a problem hiding this comment.
I need to look around, somewhere there is a rule set of which constructors getting created for free when you define other ones. Here I just meant
~complex() = default;
stan/math/fwd/core/std_complex.hpp
Outdated
| template <typename U> | ||
| complex(const U& x) : base_t(x) {} // NOLINT(runtime/explicit) |
There was a problem hiding this comment.
Should this be marked explicit? Also should this be templated to only allow scalar types with something like
template <typename U, typename = require_stan_scalar_t<U>>
explicit complex(const U& x) : base_t(x) {} // NOLINT(runtime/explicit)There was a problem hiding this comment.
It won't work if it's marked explicit. We need to be able to promote arithmetic types to complex if necessary.
It will only compile if U is something assignable to the class template T, which means the restriction is to arithmetic types (primitives) or var (or to fvar<U> for the forward version). Elsehwere, the decision has been not to tighten down the signature with type traits unless needed for ambiguity resolution.
There was a problem hiding this comment.
It won't work if it's marked explicit. We need to be able to promote arithmetic types to complex if necessary.
As is yes but if you added the operator= for scalar types it would work right?
I'm fine with it not being explicit but I'll leave this open for now until I have time to sit down and play with this / look at the spec
(also fyi my review is a comment and not a request for changes so everything here is optional)
stan/math/fwd/core/std_complex.hpp
Outdated
| * Construct a complex number with zero real and imaginary | ||
| * components. | ||
| */ | ||
| complex() : base_t() {} |
There was a problem hiding this comment.
Rule of 5? I'm pretty sure making a non-default constructor or destructor turns off the implicit move constructors
There was a problem hiding this comment.
The default constructor would have the wrong behavior. std::complex() needs to initialize the real and imaginary components to zero.
There is nothing to move, so move semantics won't do anything. Should I still define the move copy constructor and copy assignment and just define them = default?
There was a problem hiding this comment.
Should I still define the move copy constructor and copy assignment and just define them = default?
I think it's fine to add them if the compiler wants to use them and they can just be default
stan/math/fwd/core/std_complex.hpp
Outdated
| template <typename V> | ||
| complex_type& operator=(const V& x) { | ||
| return base_t::operator=(x); | ||
| } |
There was a problem hiding this comment.
Can V be literally any type? Adding a require to restrict types here may be good
There was a problem hiding this comment.
V must be assignable to the value type of the complex number. It won't compile otherwise. Adding an enable_if restriction would just cause the compiler to fail in a different way. We don't need the restriction for disambiguation.
My understanding is that we generally do not want to try to explicitly enumerate all the possible instantiations of a template parameter with type traits. We don't do that on any of the prim functions in our math library, for example. You can try to pass a std::string to log1m, but it won't compile.
stan/math/prim/core/complex_base.hpp
Outdated
| complex_type& derived_complex_ref() { | ||
| return static_cast<complex_type&>(*this); | ||
| } |
There was a problem hiding this comment.
Why not just call this derived()? I think that's common
There was a problem hiding this comment.
I didn't do that because it's not an exact use of the CRTP. It's just CRTP-like, so there's no template parameter Derived. Instead there's a template parameter T and the equivalent of derived is std::complex<T>, which is typedef-ed as complex_type.
But I'm happy to change it if you think that'd be clearer.
There was a problem hiding this comment.
I'm okay with it as is, but imo I'd rather just have a CRTP than CRTP-like.
| class complex<stan::math::fvar<T>> | ||
| : public stan::math::complex_base<stan::math::fvar<T>> { |
There was a problem hiding this comment.
If this is a crtp should this be
public stan::math::complex_base<complex<stan::math::fvar<T>>>
So the type is recurrent in the base class?
There was a problem hiding this comment.
No. The complex (which would have to be std::complex) is redundant. I only take the value type T as the template parameter, but the "derived" class is std::complex<T>. If I suppoied all of complex<fvar<T>>, then I'd just have to go fishing the T out with traits again.
There was a problem hiding this comment.
It is definitely redundant, though if I use a pattern I like keeping it standard instead of 'ish. But whether a strict CRTP pattern here would be better is totally subjective.
then I'd just have to go fishing the T out with traits again.
Would argue that type traits change here would be fishing in a barrel
using value_type = value_type_t<V>;
/**
* Derived complex type used for function return types.
*/
using complex_type = V;| template <typename X> | ||
| complex_type& operator/=(const std::complex<X>& other) { | ||
| using stan::math::square; | ||
| value_type sum_sq_im = square(other.real()) + square(other.imag()); |
There was a problem hiding this comment.
Can/should these be auto?
There was a problem hiding this comment.
What's the feeling about auto when there's a simple type we can use? I find the explicit types simpler if they're small. I'm not bothered either way, so let me know. Our existing code is all over the place on this issue.
There was a problem hiding this comment.
I personally like auto so that type deduction just rides through instead of performing an implicit cast if there is a mismatch. I'm fine either way here
|
@SteveBronder : Thanks for reviewing! I replied to all the questions. I think everything's OK as is, but there are quite a few things you suggested I'd be OK changing, so let me know which ones you want me to change after seeing why I did them the way I did. |
|
^word! Yeah consider all that optional. I can also goof around and make a branch with those things to see how they'd look |
stan/math/prim/core/complex_base.hpp
Outdated
| ~complex_base() {} | ||
|
|
||
| /** | ||
| * Return a reference to thi class cast to the derived complex |
stan/math/prim/core/complex_base.hpp
Outdated
| * | ||
| * @param[in] x the value to set the real part to | ||
| */ | ||
| void real(const value_type& x) { re_ = x; } |
There was a problem hiding this comment.
Why can't this be something that can get cast to a value_type (like a double)? Looks like the stuff here is just pass by value: https://en.cppreference.com/w/cpp/numeric/complex/real
There was a problem hiding this comment.
Yes, I can generalize that and avoid some casting to the value_type.
| using stan::math::value_of_rec; | ||
| std::complex<T> z(x, y); | ||
| EXPECT_EQ(1.1, value_of_rec(z.real())); | ||
| EXPECT_EQ(2.3, value_of_rec(z.imag())); |
There was a problem hiding this comment.
Why are these two lines hardcoded? Can't it be:
EXPECT_EQ(value_of(x), value_of_rec(z.real()));
EXPECT_EQ(value_of(y), value_of_rec(z.imag()));
stan/math/prim/meta/scalar_type.hpp
Outdated
| * @tparam T value type of complex number | ||
| */ | ||
| template <typename T> | ||
| struct scalar_type<std::complex<T>> { |
There was a problem hiding this comment.
This could be a bit weird. The complex number is the scalar type. I feel like the way this is written it should be called autodiff_scalar_type.
return_type leans on scalar_type (
math/stan/math/prim/meta/return_type.hpp
Line 33 in ed61d81
return_type<std::complex<var>, double>::type would be var, which isn't what I'd expect mathematically.
|
On Feb 19, 2020, at 3:43 PM, Steve Bronder ***@***.***> wrote:
@SteveBronder commented on this pull request.
In stan/math/fwd/core/std_complex.hpp:
> + template <typename U>
+ complex(const U& x) : base_t(x) {} // NOLINT(runtime/explicit)
It won't work if it's marked explicit. We need to be able to promote arithmetic types to complex if necessary.
As is yes but if you added the operator= for scalar types it would work right?
That won't participate in promotion for function calls. But maybe we could still get away without it. I'm a bit reluctant given that the std::complex base class uses an implicit constructor:
https://en.cppreference.com/w/cpp/numeric/complex/complex
|
|
Looks like we're going to put this PR on hold yet again while I rewrite the meta libs. Then I have to come backand fix this. So please don't merge this. |
bob-carpenter
changed the title
…re/0123-complex-spec
|
This doesn't need put on hold. It's the next pull that has all the complexity, right? And even then we can redo the metaprograms afterwards. It doesn't change anything. The only thing I'd say is this function: should accept more than a value type since apparently that makes sense and then this goes in. Like: Should work and just cast that double to a var. If this already happens and I'm just not understanding the C++, lemme know and I'll approve (and merge after tests pass). |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
|
From @SteveBronder:
The path of least resistance is to change the code so I can get this thing out before more people pile on with change requests. I'll do this:
|
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
bob-carpenter
changed the title
…gs/RELEASE_500/final)
…gs/RELEASE_500/final)
…gs/RELEASE_500/final)
…gs/RELEASE_500/final)
…re/0123-complex-spec
|
@mitzimorris it looks like this is failing because Jenkins can't find stanc in cmdstan? Do you know what's up with that? https://jenkins.mc-stan.org/job/CmdStan/job/downstream_tests/1453/console |
|
@SteveBronder noticed a bit earlier too, on it |
|
@serban-nicusor-toptal much appreciated! |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
|
@bbbales2 I'm afraid we need another review for this one after changes. There's not a huge rush here because I'm working on a branch that has already merged this. |
|
Just the constructor? For what type U? And what are you comparing it to?
…On Tue, Mar 3, 2020 at 1:29 PM Steve Bronder ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In stan/math/prim/core/complex_base.hpp
<#1720 (comment)>:
> + /**
+ * Derived complex type used for function return types.
+ */
+ using complex_type = std::complex<value_type>;
+
+ /**
+ * Construct a complex base with the specified real and imaginary
+ * parts.
+ *
+ * @tparam T real type (must be assignable to `V`)
+ * @tparam U imaginary type (must be assignable to `V`)
+ * @param[in] re real part
+ * @param[in] im imaginary part (default 0)
+ */
+ template <typename T, typename U>
+ complex_base(const T& re, const U& im) : re_(re), im_(im) {}
Yeah sure I'll close this and do something on discourse
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1720?email_source=notifications&email_token=AAZ2D7Y4JE4IEQ4RN4Z7243RFVEATA5CNFSM4KWHI7P2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCXZJSHQ#discussion_r387210955>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZ2D74YUQ7HO3TPWY23LILRFVEATANCNFSM4KWHI7PQ>
.
|
| */ | ||
| using value_type = V; | ||
| using value_type = ValueType; |
There was a problem hiding this comment.
Is our standard to do underscore_naming for usings and CamelCase for template parameters?
There was a problem hiding this comment.
That's consistent with how I understand the code but it's weird to see if directly in a situation like this :/.
|
The using descriptors should be underscore separated. The template
parameters should be capitalized.
The C++ standard library uses single capital letters for template
parameters and underscore separated lowercase for everything else. I've
always tried to follow that.
More recently, Stan devs have started using more descriptive template
parameter names which will indeed call for camel case. The standard
library doc just uses `T` for the value type of a complex number, not
`ValueType`.
|
Summary
This is the first pull request of two for adding support for autodiff of
std::complexvalues in Stan. This PR hasstd:complex<var>andstd::complex<fvar<T>>Tests
The AD test framework was extended to deal with complex numbers and used to test the member operators (compound arithmetic assignments only).
Specialized tests were implemented for the constructors and defaults.
The tests for std::complex classes are in
test/unit/math/mix/core/std_complex_test.cpp. Tests for additions to the testing framework itself are in the testing framework attest/unit/math/.Side Effects
No
Checklist
Math issue complex numbers with vars #123 [but this doesn't solve the whole issue, this is just PR 1 of 2 to make reviewing easy]
Copyright holder: (fill in copyright holder information): Columbia University
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 (I only unit tested new additions; test-math-dependencies is broken)
./runTests.py test/unit)make test-headers)make test-math-dependencies)make doxygen)make cpplint)the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested