Exposing MaxMagnitudeNumber, MaxNumber, MinMagnitudeNumber, and MinNumber #68217
Conversation
…e relevant scenarios
…s into AssertExtensions
…er, and MinNumber
|
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 isn't taking into account #68191 yet, that will happen as part of a separate work item after the next API review.
|
dakersnar
left a comment
dakersnar
left a comment
There was a problem hiding this comment.
Implementation looks good, skimmed the tests and they look good as well.
| double ax = Abs(x); | ||
| double ay = Abs(y); | ||
|
|
||
| if ((ax > ay) || IsNaN(ay)) |
There was a problem hiding this comment.
Two questions:
- If ax was NaN and ay was 1.23, would the "(ax > ay)" condition return false? EDIT: Scratch this, you have a test case that confirms this behavior.
- If both ax and ay are NaN, NaN (ax) would be returned here. Does that contradict with the comment "It does not propagate NaN inputs back to the caller"?
There was a problem hiding this comment.
- Yes,
x cmp yalways returnsfalseif eitherxoryis NaN - No, in the case that both are
NaN,NaNis the thing to return
| /// <param name="y">The value to compare with <paramref name="x" />.</param> | ||
| /// <returns><paramref name="x" /> if it is greater than <paramref name="y" />; otherwise, <paramref name="y" />.</returns> | ||
| /// <remarks>For <see cref="IFloatingPoint{TSelf}" /> this method matches the IEEE 754:2019 <c>maximumNumber</c> function. This requires NaN inputs to not be propagated back to the caller and for <c>-0.0</c> to be treated as less than <c>+0.0</c>.</remarks> | ||
| static abstract TSelf MaxNumber(TSelf x, TSelf y); |
There was a problem hiding this comment.
The docs on this are identical to the docs on MaxMagnitudeNumber, with the sole exception of saying that it matches maximumNumber instead of maximumMagnitudeNumber, but for that to be meaningful to me as a consumer I need to know what IEEE says those are :) Can we be more explicit in the summary about the comparison being performed in order to differentiate these?
Same for the comments below.
There was a problem hiding this comment.
It also differs on This requires NaN inputs to be propagated vs This requires NaN inputs to not be propagated
I'll submit a follow up PR to update the docs to be more explicit in the summary.
There was a problem hiding this comment.
Actually, I'll update them in this PR. This isn't going to make the snap since Half.op_Modulus is still failing in CI only
There was a problem hiding this comment.
They both say "This requires NaN inputs to not be propagated back to the caller". Is that just a typo then?
There was a problem hiding this comment.
Oh sorry, I misread this as saying MaxNumber and Max docs were identical.
I did fix it up so Max, MaxNumber, and MaxMagnitudeNumber all differ in their docs
There was a problem hiding this comment.
Basically:
Maxreturns the greater ofxvsy, preferringNaNwhen presentMax(2, -3)returns2Max(2, NaN)returnsNaN
MaxMagnitudereturns the greater ofxorycomparing using their absolute value (magnitude), preferringNaNwhen presentMaxMagnitude(2, -3)returns-3MaxMagnitude(2, NaN)returnsNaN
MaxNumberreturns the greater ofxvsy, not preferringNaNwhen presentMaxNumber(2, -3)returns2MaxNumber(2, NaN)returns2
MaxMagnitudeNumberreturns the greater ofxorycomparing using their absolute value (magnitude), not preferringNaNwhen presentMaxMagnitudeNumber(2, -3)returns-3MaxMagnitudeNumber(2, NaN)returns2
…r to clarify it doesn't propagate NaN
…mber (dotnet#68217) * Exposing MaxMagnitudeNumber, MaxNumber, MinMagnitudeNumber, and MinNumber * Ensure Max, MaxMagnitude, Min, and MinMagnitude tests are covering the relevant scenarios * Moving the floating-point AssertEqual helper from the Math/MathF tests into AssertExtensions * Adding tests covering MaxMagnitudeNumber, MaxNumber, MinMagnitudeNumber, and MinNumber * Disabling a few Half tests due to an issue around -0 * Disabling a few Half tests due to an issue around +0 * Fixing 67993 as the test helper had a bug where it used the wrong target type * Fixing up the doc comment for Min/MaxMagnitudeNumber and Min/MaxNumber to clarify it doesn't propagate NaN
ghost
locked as resolved and limited conversation to collaborators
3 participants
This isn't taking into account #68191 yet, that will happen as part of a separate work item after the next API review.