Skip to content
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

Complex number support #567

Merged
merged 29 commits into from Mar 31, 2019
Merged

Complex number support #567

merged 29 commits into from Mar 31, 2019

Conversation

sebcrozet
Copy link
Member

This adds complex number support almost everywhere a Real was accepted before. This includes decomposition of complex matrices as well as BLAS operations involving conjugate-transposition.

This relies on ongoing work on the alga crate on the complex branch.

Fix #281
Fix #503

@sebcrozet sebcrozet added enhancement P-medium Medium priority breaking change Fixing this issue, or merging this pull-request is likely to require breaking changes labels Mar 23, 2019
@sebcrozet sebcrozet changed the base branch from master to dev March 23, 2019 11:05
@sebcrozet sebcrozet mentioned this pull request Mar 23, 2019
@sebcrozet sebcrozet added this to the v0.18 milestone Mar 23, 2019
@Coder-256
Copy link

Wow this is an enormous PR, I totally underestimated the magnitude of this change. Thank you for all this, it will definitely be a huge help in my project and surely many others!

That being said, I have just a few questions. For many of these functions that now use Complex, wouldn't that be a breaking change since people now have to wrap everything with Complex::new(n, 0)? Or are there different functions that take complex numbers? (I'm finding it hard to figure this out myself through all the code; especially because the documentation won't build). Additionally, I am concerned about the implications of this on memory allocation if now you need to allocate double the memory for unused fields. On the topic of type safety, what happens to the return type functions that return real values for real inputs and complex values for complex inputs, such as the trigonometric functions? (or even simple linear functions; in fact probably most functions). For example, I want sin(a) to return a real value rather than having to assert that a complex return has no imaginary part. Lastly, it is really hard to read this behemoth of a diff even when cherry-picking, so would it be possibly to very briefly sum up what has changed?

Again, I really appreciate the open-source work that went into this project and it will definitely be useful no matter what! 😄

@Ralith
Copy link
Collaborator

Ralith commented Mar 23, 2019

For many of these functions that now use Complex, wouldn't that be a breaking change since people now have to wrap everything with Complex::new(n, 0)?

Complex is a trait, not a struct, and per dimforge/alga@0576382 every type that implements Real (e.g. f32) also implements Complex.

It would be interesting to see how well the optimizer copes with the increased abstraction, though.

@Coder-256
Copy link

@Ralith Thank you, I see you're right, I did not realize there was a new trait because I was mixing it up with the Complex struct from num_complex. That is a clever way to implement things. However that now adds even more confusion to my question about return types.

@Ralith
Copy link
Collaborator

Ralith commented Mar 23, 2019

I think everything's continuing to return the same type it's given as input, so e.g. if you want to get complex results for real inputs you'll have to lift your reals into a complex type with an explicit zero imaginary component first yourself.

@Coder-256
Copy link

@Ralith What I mean is, if we now change the sin function to accept any type conforming to Complex, then what is the return type? Is it the type that I pass it? In that case, what about functions that actually need to return a complex number regardless of whether the input is real or not? (ex. finding the square root of a negative real)

@Ralith
Copy link
Collaborator

Ralith commented Mar 23, 2019

Is it the type that I pass it?

I think so.

In that case, what about functions that actually need to return a complex number regardless of whether the input is real or not?

For types that cannot represent the imaginary component, they'll probably do the same thing they do today. If you don't want that behavior, use a type that can represent the imaginary component.

@sebcrozet
Copy link
Member Author

@Coder-256 I confirm what @Ralith said. Everything will work just like before for anything that are not complex numbers. If you use f64, you will get operations on real matrices and vectors that return the same float-based results as before. The novelty here is that if you want to use num_complex::Complex, you can, and it will perform all the operations on the complex field.

What I mean is, if we now change the sin function to accept any type conforming to Complex, then what is the return type? Is it the type that I pass it?

If you have a function that takes a type N: Complex, and outputs N, then the return type is the same as the input type. If you have N = f64, it will output a f64. If N = num_complex::Complex<f64>, it will output a num_complex::Complex<f64>. One particularity though is that some operations, like the modulus, take a complex number and return a real number. In that case the signature looks like this: modulus<N: Complex>(val: N) -> N::Real where ::Real is an associated type provided by the new Complex trait.

In that case, what about functions that actually need to return a complex number regardless of whether the input is real or not? (ex. finding the square root of a negative real)

The square root of a negative real does not exist if you are not working on the complex field. So for this square root to exist, you need to provide a complex number (with an imaginary part equal to zero) in the first place.

I understand having a trait named Complex can be quite confusing. Would you @Coder-256 or @Ralith, or perhaps @jswrenn have a better name idea?

@jswrenn
Copy link
Sponsor Contributor

jswrenn commented Mar 24, 2019

Name collisions between different numeric libraries are probably inevitable. I think Complex is an appropriate name. I think referring to it in the docs as alga::Complex (rather than just Complex) would clear up most of the confusion.

(This would probably be a good thing to do with other traits that come from alga, too.)

@sebcrozet
Copy link
Member Author

@jswrenn Thank you for your input.
@WaDelma You contributed a lot to alga too, do you have an opinion about whether or not we should name Complex the alga trait for complex numbers or if another name would be more suitable?

@Coder-256
Copy link

@sebcrozet IMHO naming the trait Complex is a bit confusing. I initially thought that it was meant only for complex numbers. How about ComplexField? That implies that it accepts any numeric type in the complex field (C). Maybe RealField too for consistency. (just an idea)

@sebcrozet
Copy link
Member Author

@Coder-256 Thank you for the suggestions, I think those are actually great names!

@WaDelma
Copy link
Sponsor

WaDelma commented Mar 26, 2019

ComplexField sounds good to me.

# Conflicts:
#	src/base/ops.rs
#	src/geometry/isometry.rs
#	src/geometry/quaternion.rs
#	src/geometry/quaternion_construction.rs
#	src/geometry/rotation.rs
#	src/geometry/similarity.rs
#	src/geometry/transform.rs
#	src/geometry/translation.rs
#	src/geometry/unit_complex.rs
…enchmarks.

We are forced to remove the dependency because of the Cargo bug rust-lang/cargo#4866 that would break compilation for #[no-std].

In practice, this means benchmarks will not compile any more unless we manually uncomment the criterion dependency.
@sebcrozet sebcrozet merged commit 31bc336 into dev Mar 31, 2019
@sebcrozet sebcrozet deleted the complex branch March 31, 2019 12:44
@adamnemecek
Copy link
Contributor

U go gurl!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Fixing this issue, or merging this pull-request is likely to require breaking changes enhancement P-medium Medium priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants