-
Notifications
You must be signed in to change notification settings - Fork 348
Add phase angle calculation functions for complex arrays #1543
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
base: master
Are you sure you want to change the base?
Conversation
https://en.wikipedia.org/wiki/Argument_(complex_analysis) Implement phase angle (argument) calculation for complex numbers in arrays, providing NumPy-compatible functionality. The angle represents the phase of a complex number in the complex plane, calculated as atan2(imaginary, real). Features: - Calculate phase angles in range (-π, π] for radians, (-180°, 180°] for degrees - Support for real numbers (f32, f64) and complex numbers (Complex<f32>, Complex<f64>) - Two API variants: - NumPy-compatible: always returns f64 regardless of input precision - Precision-preserving: maintains input precision (f32 → f32, f64 → f64) - Both array methods and standalone functions available - Proper handling of signed zeros following NumPy conventions: - angle(+0 + 0i) = +0, angle(-0 + 0i) = +π - angle(+0 - 0i) = -0, angle(-0 - 0i) = -π API additions: - ArrayRef::angle(deg: bool) -> Array<f64, D> - ArrayRef::angle_preserve(deg: bool) -> Array<InputType, D> - ndarray::angle(array, deg) -> Array<f64, D> - ndarray::angle_preserve(array, deg) -> Array<InputType, D> - ndarray::angle_scalar(value, deg) -> f64 All functions tested, feature-gated with std and include documentation.
I'm not sure about this. I think everything we do in |
Yes it is. So the motivation for adding the angle functionality was because I was working on porting some Python and Numpy code to Rust and ndarray and I wanted to make sure I was matching the functionality.
This makes sense and was not something I was aware of (first contribution and all that). I have absolutely no ties/explicit need to always promote to f64 and am happy to remove it from the PR. However, on this it might be good to explicitly mention details like this, even in just the ndarray for numpy users docs. You know make it clearer that the crate does not aim to emulate numpy functionality, even if it was just in a contributions guideline doc. Provided I remove this how does it seem? Any other concerns? Happy to answer |
|
My opinion is that we shouldn't be doubling our API with "always returns f64 regardless of input precision" functions. If we do it for phase angle, why not for abs, floor, etc.? Moreover, let's say we have to do it for a specific case, I'd name the "preserving" function normally and add a suffix to the non-preserving function, something like
Your MR is quite clean! Thank you for contributing. I usually ask for a test when people are adding functions. Nothing big, but at least one test. |
|
I have created a new commit which should address your concerns. There is no longer the HasAngleF64 trait and no more type promotion to f64. The user controls the float type. Then I added tests to cover the angle functionality |
src/numeric/impl_float_maths.rs
Outdated
| /// ``` | ||
| #[cfg(feature = "std")] | ||
| #[cfg_attr(docsrs, doc(cfg(feature = "std")))] | ||
| pub fn angle<A, F, S, D>(z: &ArrayBase<S, D>, deg: bool) -> Array<F, D> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is angle defined again? Wasn't the definition above sufficient to use it?
akern40
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your PR! I've left a few specific comments below. One top-level question for you and @nilgoyette: do we want to use deg: bool as a function argument? Two alternatives would be to have angle_deg and angle_rad functions, or simply just stick to radians and let users convert to degrees as-needed. I am personally leaning towards the last option for three reasons:
- Since Rust doesn't have keyword arguments, the call-sites right now will look like
angle(true)andangle(false). These don't immediately tell you what kind of return values to expect; the other two options do. I think this is more inline with the Rust tendency towards explicit over implicit semantics. - The Rust std library does everything in radians and provides a
to_degreesfunction; we've generally followed this pattern withcos,sin, etc and it would be good to stay consistent. - Much less important: the use of an
ifstatement inside the function call means that we're forcing a branch inside the caller's code. This is a micro-optimization, but I would rather leave it up to the user to call the function that they need.
Please let me know if you have any questions - we always appreciate contributors to the library, and are glad you're finding it useful in your work!
src/numeric/impl_float_maths.rs
Outdated
| pub trait HasAngle<F: Float + FloatConst> { | ||
| /// Return the phase angle (argument) in the same precision as the input type. | ||
| fn to_angle(&self) -> F; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This trait can be written without a generic:
pub trait HasAngle {
type Angle: Float + FloatConst;
fn to_angle(&self) -> Self::Angle;
}
this will also let you remove the trait's generic in the implementations, and remove the generic on ArrayRef::angle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, love learning new things like the Angle stuff.
src/numeric/impl_float_maths.rs
Outdated
| /// ``` | ||
| #[cfg(feature = "std")] | ||
| #[cfg_attr(docsrs, doc(cfg(feature = "std")))] | ||
| pub fn angle_scalar<F: Float + FloatConst, T: HasAngle<F>>(z: T, deg: bool) -> F |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ndarray does not generally consider it in-scope to support free functions for scalar values. This function sits on top of to_angle by performing the deg/rad conversion; could it be moved to be an auto-implemented function on the HasAngle trait?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(That is, if we keep the deg: bool argument at all).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll check this out but also see my response under the main comment on the deg stuff.
src/numeric/impl_float_maths.rs
Outdated
| let mut result = self.map(|x| x.to_angle()); | ||
| if deg { | ||
| result.mapv_inplace(|a| a * F::from(180.0).unwrap() / F::PI()); | ||
| } | ||
| result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to use deg: bool as an argument, I would recommend wrapping these lines in an if-else statement and including the degree conversion inside the initial map. Otherwise, without compiler optimizations, this will perform two scans of the array (although mapv_inplace is at least nice in that it avoids two allocations).
|
I agree with the reasoning for keeping everything in radians (option 3). The original reference point for this PR was the NumPy function, which included a deg parameter, but I now see that the Rust ecosystem and ndarray follow the standard library convention of using radians throughout. The main takeaway from this PR, for me, is not the phase-angle functionality itself but how it has surfaced several (seemingly) unwritten conventions in the codebase. Most of the feedback (all the feedback is very welcome) has not been about the actual calculation logic, but about implicit stylistic and structural expectations. Even the discussion around the added traits has focused on precision and consistency rather than implementation details. To be clear, I don't think this is bad, it's really good. It's about focusing on the actual design and trying to maintain a cohesive system. As a first-time contributor, it would have been super helpful to have a clearer written guidance on these conventions since much of this knowledge currently seems to exist only through review comments. Edit: mostly just organising and formalising my thoughts |
|
And I am also aware that adding such guidelines is not an easy task and relies on a lot of "in-house" knowledge that may not be easily surfaced. But just wanted to give my thoughts on the matter. I still plan on contributing more to ndarray (if you'll have me) and I look forward to reading your thoughts on this. |
|
Totally heard! Contributor guidelines - and generally better and more beginner-friendly documentation - are on the forefront of my mind. I appreciate both your willingness to take feedback and the way you've surfaced your experiences and challenges as a first time (and hopefully repeating!) contributor. If you're willing to entertain me, would you mind telling me a bit more about how your background with NumPy shaped your expectations when contributing for the first time? My impression is that this is a very common scenario, and one we might want to explicitly address. It's an excellent source of developers that I'd like to welcome, but there are many differences about how |
|
No problem at all. I really appreciate the engagement. I love the crate, it’s essential, and I’ve recently been using it more heavily as the core data structure in my audio library. In hindsight, I can see that I fell into the trap of direct translation. I tried to port functionality one-to-one without fully reconsidering how it should be realised idiomatically in Rust. Python often uses the “pass all the args and kwargs” pattern for flexibility, while Rust favours composing small, precise functions. The radians-versus-degrees issue is a good example: in Python, you’d naturally include a The main takeaways for me are:
For example, even something as simple as the following paragraph early in the docs would make a big difference: “ If you ever do formalise this, I think a few targeted pages would go a long way: a short note on porting NumPy to idiomatic Rust, a summary of core conventions ( Due to the nature of the crate and the proliferation of the NumPy ecosystem people will always conflate the two. There's no just way around it. More and more developers, researchers and hobbyists are looking at Rust for performance and stability and when they look for the ndimensional arrays they're so used to in NumPy they will land on Also love the idea of a blog post on this. If you want a co-author or even just a proof reader I'm happy to help. I love all these small non-code details of making software simple to use. |
arguments - Tidied up the comments/documentation to remove all traces of the originally version of the PR which included float promotions. - Removed the duplicate angle function - Removed the deg parameter in the various functions. This leaves the conversion up to the user and can easily be achieved with the Float::to_degrees function - Tidied some trait bounds which did not need the FloatConst trait - Ran ``cargo fmt``
|
I have pushed the next commit which hopefully addresses the comments. If not just let me know. |
https://en.wikipedia.org/wiki/Argument_(complex_analysis)
Implement phase angle (argument) calculation for complex numbers in arrays, providing NumPy-compatible functionality. The angle represents the phase of a complex number in the complex plane, calculated as atan2(imaginary, real).
Features:
API additions:
All functions tested, feature-gated with std and include documentation.