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

Updating the generic math design doc to account for additional changes #262

Merged
merged 35 commits into from
May 11, 2022

Conversation

tannergooding
Copy link
Member

This namely covers usage of DIMs and gives additional sample code showing how DIMs might be used in various functions. This helps show the layering is "sound" and gives some examples of how future changes might be implemented when that becomes relevant.

@tannergooding tannergooding requested a review from dakersnar April 28, 2022 21:23
@tannergooding
Copy link
Member Author

tannergooding commented Apr 28, 2022

I'd recommend reviewing the doc "as a whole", rather than only looking at the changes in this PR.

Noting this is also not "complete" and will build up some more changes (several APIs need to move down to INumberBase still) before the API review on May 10th

return y;
}

public static virtual TSelf MaxMagnitudeNumber(TSelf x, TSelf y)
Copy link
Member Author

Choose a reason for hiding this comment

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

FYI. @stephentoub that this includes the changes necessary for LINQ Min/Max.

Copy link

@dakersnar dakersnar left a comment

Choose a reason for hiding this comment

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

I read through the file and skimmed the code parts. I really like the "Scenarios" section. Seems like there is more to do but your additions in this PR look mostly mechanical. Out of curiosity, when you make updates like this, are you copy pasting code into this file, or is it auto generated somehow?

Couple spelling mistakes I happened to spot:

"...Jeff is trying to compute the standard deviation of a list of numbers. They are surprised to find that such a function isn't built into .NET. After searching the web, Jeff finds a blog post with the following method..."

  • Change "they" to "he" to be consistent with later pronouns, I think.

"…that allow Jeff to implement the desired method without requiring users types to take a dependency on types they…"

  • Add a ' after "users"?

"we need to strike a balance between expressiveness, exxtensibility, and cost."

  • Typo: "exxtensibility"

@terrajobst
Copy link
Contributor

  • long GetSignificandShortestBitLength() should return int
  • Let's bias towards explicit implementations on the primitives, especially for concepts that generally only make in the generic context (or already exist under a different name)
  • Char, CLong, CULong should probably implement everything explicitly
  • We don't plan to implement these interfaces outside the BCL (e.g. the core types and types in System.Numerics)
    • For example, the SQL types
  • INumberConverter<T, U>
    • Should have three methods, ConvertChecked, ConvertUnchecked, ConvertTruncated
    • No try versions

@tannergooding
Copy link
Member Author

Will update the design doc according to the feedback to ensure it stays up to date/correct and then will merge

@tannergooding
Copy link
Member Author

Updated doc according to the feedback. Noting that that the Parse methods aren't DIM because TryParse only returns bool and that isn't enough to disambiguate between overflow and format failures

@tannergooding tannergooding merged commit ac05672 into dotnet:main May 11, 2022
@govert
Copy link

govert commented Jun 10, 2022

@tannergooding
It would be very helpful to add a member like IFloatingPoint.MachineEpsilon or IFloatingPoint.EpsilonUnitRoundoff into these interfaces. This will help address a very common error when porting numerical code to .NET. I think a mistake was made when the .NET BCL defined double.Epsilon, float.Epsilon etc. as ‘the smallest representable value that is greater than zero’. In other languages and most computation arithmetic, the value of ‘epsilon’ is given by the ‘Machine Epsilon’ as the difference between 1 and the next larger floating point number. See https://en.wikipedia.org/wiki/Machine_epsilon . Also compare the definition of std:numeric_limits::epsilon in C++ https://en.cppreference.com/w/cpp/types/numeric_limits/epsilon Similar for FORTRAN: https://gcc.gnu.org/onlinedocs/gfortran/EPSILON.html

So the name ‘epsilon’ is used for a crucial constant that has a different value in most numerical algorithm settings than in the .NET CLR. It's not easy to discover this difference and it may have significant impact in an algorithm.

Obviously one can’t go back to fix this 'mistake' in the .NET BCL, but the numerical interfaces seem like a good opportunity to help mitigate this issue a bit. One question might relate to the naming – it would be nice to have a name that points in the right direction, yet is easy to discover in a member list, e.g. if it starts with ‘Epsilon’ as the prefix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants