-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Fix Tensor.StdDev vs TensorPrimitives.StdDev differences for Complex input #119229
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
Conversation
…nces for Complex input Co-authored-by: jeffhandley <[email protected]>
…input Co-authored-by: jeffhandley <[email protected]>
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.cs
Outdated
Show resolved
Hide resolved
…tion - Fix typo: "TensorPrimitivers" -> "TensorPrimitives" in test comment - Replace custom SumOfSquaredMagnitudeDifferences operation with direct use of TensorPrimitives.StdDev - Leverage TensorPrimitives vectorization optimizations where T.Abs is only called when necessary - Remove unnecessary custom operation that always called T.Abs for all types - Use TryGetSpan for contiguous tensors and fallback to FlattenTo for non-contiguous tensors Co-authored-by: jeffhandley <[email protected]>
[Fact] | ||
public static void TensorStdDevComplexTests() | ||
{ | ||
// Test case from issue: Tensor.StdDev vs TensorPrimitivers.StdDev differences for Complex input |
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.
Fixed the typo in commit 4585313.
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/TensorOperation.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.cs
Outdated
Show resolved
Hide resolved
…lines Co-authored-by: jeffhandley <[email protected]>
…or non-essential methods and convert StdDev tests to Theory tests Co-authored-by: jeffhandley <[email protected]>
Added comment to clarify absolute value usage for complex numbers.
@tannergooding I explored a couple options on this and this seems best to me. For my own reference/learning, I looked into how NumPy handles complex numbers for Euclidean Distance and Standard Deviation of complex numbers and found it also resorts to an absolute value of the diff for StdDev but not Distance, increasing my confidence this was the correct approach. I do worry slightly about introducing the Abs call purely to support complex numbers, but I couldn't find a way to distinguish between real and (custom) complex numbers reliably from the interfaces. If you have a trick up your sleeve here, that could change the implementation. |
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.
Pull Request Overview
This PR fixes mathematical correctness issues between Tensor.StdDev
and TensorPrimitives.StdDev
when calculating standard deviation for complex numbers. The core problem was that the tensor implementation was squaring complex differences directly instead of taking the absolute value before squaring, which is required for mathematically correct standard deviation calculation.
Key changes:
- Introduced
SumOfSquaredMagnitudeDifferences<T>
operation that properly computesT.Abs(x - mean)
before squaring - Updated
Tensor.StdDev
to use the new operation for consistent results withTensorPrimitives.StdDev
- Enhanced test coverage with Theory tests covering both real and complex number scenarios
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/libraries/System.Numerics.Tensors/tests/TensorTests.cs |
Converts existing tests to Theory tests with comprehensive test cases for both float and complex numbers, adds TestComplex struct for testing |
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/TensorOperation.cs |
Adds new SumOfSquaredMagnitudeDifferences operation that correctly applies absolute value before squaring |
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.cs |
Updates StdDev implementation to use the new mathematically correct operation |
Co-authored-by: Copilot <[email protected]>
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/TensorOperation.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/TensorOperation.cs
Show resolved
Hide resolved
/backport to release/10.0 |
Started backporting to release/10.0: https://github.com/dotnet/runtime/actions/runs/17786818289 |
Fixes #116133
Core Fix
SumOfSquaredAbsoluteDifferences<T>
operation that computesT.Abs(x - mean)
before squaringTensor.StdDev
to use this new operation, ensuring mathematically correct results for complex numbersTest Improvements
TensorStdDevTests
toTensorStdDevFloatTests
Theory test with multiple test cases including sequential numbers, same values, and mixed positive/negative valuesTensorStdDevComplexTests
to Theory test with test cases for complex numbers from the original issue, purely imaginary numbers, and real complex numbersTensorStdDevNonContiguousTests
to ensure non-contiguous tensor calculations work correctlyTestComplex
struct that throwsNotImplementedException
for interface methods not essential for StdDev calculationsTest Coverage
The new Theory tests cover:
|x|² = x²
for the standard deviation calculation|z|² ≠ z²
, ensuring the fix works correctlyBoth
TensorPrimitives.StdDev
andTensor.StdDev
now return identical mathematically correct results for all test cases.