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

Complexes #350

Merged
merged 10 commits into from
Dec 31, 2021
Merged

Complexes #350

merged 10 commits into from
Dec 31, 2021

Conversation

andy-thomason
Copy link
Contributor

@andy-thomason andy-thomason commented Dec 28, 2021

This PR implements the Complexes type and provides complex number support via num_complex.

Rcplx wraps num_complex::Complex<f64> the same way that Rfloat wraps f64.

fn mycplx(x: Rcplx) -> Rcplx {
  -x
}

fn mycplxvec(x: Complexes) -> Complexes {
  x
}

fn mycplxvec2() -> Complexes {
  Complexes::from_values([(0., 0.), (1., -1.)])
}

This PR provides only basic support.

use std::ops::{AddAssign, DivAssign, MulAssign, SubAssign};

// #[cfg(feature="num-complex")]
pub type C64 = num_complex::Complex<f64>;
Copy link
Member

Choose a reason for hiding this comment

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

I know it is against the naming convention, but shouldn't it be c64 inline with f64 and i64/u64?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rust likes types to start with a capital letter. But we can override this with a #[allow()]

Copy link
Member

Choose a reason for hiding this comment

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

I know, I am just curious and asking for opininons. I am ok with either options.

@@ -197,6 +199,7 @@ make_conversions!(S4, ExpectedS4, is_s4, "Not a S4 type");
make_conversions!(Integers, ExpectedInteger, is_integer, "Not an integer type");
make_conversions!(Logicals, ExpectedLogical, is_logical, "Not a logical type");
make_conversions!(Doubles, ExpectedReal, is_real, "Not a floating point type");
make_conversions!(Complexes, ExpectedComplex, is_complex, "Not a complex type");
Copy link
Member

Choose a reason for hiding this comment

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

Maybe 'complex number'? 'Complex type' can be misleading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, next step is to ditch FromRobj

@andy-thomason
Copy link
Contributor Author

Are you good with this @clauswilke @yutannihilation @Ilia-Kosenkov ?

@clauswilke
Copy link
Member

I gave this a quick look and it generally seems fine to me. One issue that caught my eye is Rcomplex vs. Rcplx. It's not immediately clear to me why there are two types, which one an enduser would be supposed to use, and whether the difference is documented. I only found some documentation for Rcplx.

If there is a good reason for having these two types, then I think this should be documented somewhere. Also, users should be told which one to use when.

@andy-thomason
Copy link
Contributor Author

One issue that caught my eye is Rcomplex vs. Rcplx.

Currently, we either wrap num_complex or a simple struct with the same layout in Rcplx.

num_complex gives us complex arithmetic, functions, nan tests and more but
is not the same as Rcomplex even though it has the same layout.

We could have implemented arithmetic for Rcomplex, but it would have been a lot
more work and would involve a lot of transmute calls.

@clauswilke
Copy link
Member

Currently, we either wrap num_complex or a simple struct with the same layout in Rcplx.

That's fine, and I have no concerns with the technical aspects, but I think this should be stated clearly in the docs somewhere.

If I understand correctly, endusers would never normally use Rcplx, only Rcomplex. So I would suggest adding these thoughts to the documentation:

  • In the documentation for Rcplx, mention that endusers would typically use Rcomplex.
  • In the documentation for Rcomplex, mention that it wraps two different types, depending on use case. Also mention how this can be configured via features.

Copy link
Contributor

@yutannihilation yutannihilation left a comment

Choose a reason for hiding this comment

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

I'm not very familiar with complex math, but this generally looks good to me.

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.

None yet

4 participants