Exposing Radix and the remaining Is* generic-math APIs
#69651
Conversation
ghost
assigned
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
|
Tagging subscribers to this area: @dotnet/area-system-numerics Issue DetailsThis covers the last of the "new APIs". After this is just the PR to refactor the
|
| { | ||
| return *(Half*)&value; | ||
| } | ||
| public static unsafe Half Int16BitsToHalf(short value) => UInt16BitsToHalf((ushort)(value)); |
There was a problem hiding this comment.
| public static unsafe Half Int16BitsToHalf(short value) => UInt16BitsToHalf((ushort)(value)); | |
| public static unsafe Half Int16BitsToHalf(short value) => UInt16BitsToHalf((ushort)value); |
dakersnar
left a comment
dakersnar
left a comment
There was a problem hiding this comment.
Looks good! Have some comments about the design of IsComplex and IsImaginary, as well as some other small suggestions. If my thoughts have been discussed already in API reivew, let me know.
| /// <summary>Determines if a value represents a complex value.</summary> | ||
| /// <param name="value">The value to be checked.</param> | ||
| /// <returns><c>true</c> if <paramref name="value" /> is a complex number; otherwise, <c>false</c>.</returns> | ||
| /// <remarks>This function returns <c>false</c> for a complex number <c>a + bi</c> where <c>b</c> is zero.</remarks> |
There was a problem hiding this comment.
The implementation doesn't match this description, as you also return false if a is zero.
There was a problem hiding this comment.
In general for IsComplex, IsImaginary can you elaborate on the thought process for why you are handling a or b being zero in the way that you are? What is the intended use patterns for these APIs?
Assume I'm a user with an array of generic numbers, two of which are array[0]: 0 + 1i and array[1]: 1 + 1i. I'm querying for complex or imaginary on my array. Do I really expect IsComplex(array[0]) == false, or IsImaginary(array[1]) == false?
Looking to google, it seems like complex is typically defined as true for a and/or b being 0. Additionally, there is a distinction between pure imaginary numbers (what we seem to be calling imaginary in this interface) and imaginary numbers (pure imaginary numbers that can have a real component). I understand that if IsComplex followed this logic it wouldn't be very useful, as it would return true for every input besides NaN, but I'm wondering if the current behavior is confusing.
There was a problem hiding this comment.
The implementation doesn't match this description, as you also return false if a is zero.
This is likely a copy/paste issue.
The premise is that a complex number has both a real and imaginary part. That is, it is represented as a + bi. However, it's not useful as an API to only return "true" since all numbers are representable (at least in principle) as some complex number. This leads to 3 concepts: IsReal, IsImaginary, and IsComplex
A real number is some number that only has a real part. That is, its imaginary part is 0.
An imaginary number is some number that only has an imaginary part. That is, its real part is 0
A complex number is some number that has both a real and imaginary parts. That is, neither its real or imaginary parts are 0
This quantifies a + 0i as real and 0 + bi as imaginary. This also means 0 is both real and imaginary (which it is) and that all other values are complex
There was a problem hiding this comment.
This allows users to special-case pure real, pure imaginary, and true complex values as appropriate in their API surface.
These are often the three scenarios that you have to specialize within Complex itself and more complicated algorithms.
There was a problem hiding this comment.
That makes sense. As long as this behavior is properly documented, I think it should be fine.
There was a problem hiding this comment.
Going to log an issue tracking updating this as a post cleanup item to avoid spinning CI again.
| static bool INumberBase<Complex>.IsCanonical(Complex value) => true; | ||
|
|
||
| /// <inheritdoc cref="INumberBase{TSelf}.IsComplexNumber(TSelf)" /> | ||
| public static bool IsComplexNumber(Complex value) => (value.m_real != 0.0) && (value.m_imaginary != 0.0); |
There was a problem hiding this comment.
As mentioned above, this does not match the description in INumberBase.
dakersnar
left a comment
dakersnar
left a comment
There was a problem hiding this comment.
Approving assuming you address above questions
3 participants
This covers the last of the "new APIs". After this is just the PR to refactor the
Create*APIs to match API review feedback.