AdamWhiteHat / BigDecimal

An arbitrary-precision decimal (base 10) floating-point number class.
MIT License
47 stars 15 forks source link

Subtraction having issues. #5

Closed BinkersSoft closed 2 years ago

BinkersSoft commented 2 years ago

Firstly, I want to say this is an excellent library and does exactly what I need. And it is much faster than other implementations I have looked at.

Description Depending on the number of decimal places present in two BigDecimal numbers, subtraction will fail. I took several screenshots of both the code and local watches with similar numbers but with different amount of decimals. Sometimes subtraction would work and sometimes it wouldn't.

I have not seen any issues with multiplication, division, or addition. I have not tried testing any other functions as I currently don't need them.

Error/Exception Message Was an exception thrown? If so, please paste the full exception message here. Please include the full stacktrace if you have it. If you don't have a stacktrace, but you think you can reproduce the bug, please run the application in debug mode and trigger the exception again. A full stacktrace should be available to debug builds, but probably not for release builds. Otherwise, just give us a much of the error message or exception message as you can here. Please provide the error message in full and verbatim.

Expected Behavior Receive correct result, consistently.

Actual Behavior Expected behaviour was intermittent, with failures easy to reproduce.

Steps To Reproduce Should be clear in the screenshots.

Screenshots https://i.imgur.com/wtVicag.png https://i.imgur.com/pFWQduO.png https://i.imgur.com/qSsLwdP.png https://imgur.com/2bNXrwC.png https://i.imgur.com/wT1L0gv.png https://i.imgur.com/ulCRTwR.png https://i.imgur.com/IlcsV9W.png https://i.imgur.com/2qt9hst.png

Additional context/Comments/Notes/Rants Windows 10 x64 21H2 Build 19044.1645 Visual Studio Community 2022 17.1.4

Exe net6.0 disable enable x64

I have a collection of .Net SDKs and Runtimes installed from using multiple versions of Visual Studio over the years. I will try cleaning them up and see if that solves the issue.

C:\Windows\system32>dotnet --info .NET SDK (reflecting any global.json): Version: 7.0.100-preview.3.22179.4 Commit: c48f2c30ee

Runtime Environment: OS Name: Windows OS Version: 10.0.19044 OS Platform: Windows RID: win10-x64 Base Path: C:\Program Files\dotnet\sdk\7.0.100-preview.3.22179.4\

Host (useful for support): Version: 7.0.0-preview.3.22175.4 Commit: 162f83657c

.NET SDKs installed: 6.0.104 [C:\Program Files\dotnet\sdk] 6.0.300-preview.22204.3 [C:\Program Files\dotnet\sdk] 7.0.100-preview.3.22179.4 [C:\Program Files\dotnet\sdk]

.NET runtimes installed: Microsoft.AspNetCore.All 2.1.30 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All] Microsoft.AspNetCore.App 2.1.30 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 3.1.23 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 3.1.24 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 5.0.15 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 5.0.16 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 6.0.3 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 7.0.0-preview.3.22178.4 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.NETCore.App 2.1.30 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 3.1.23 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 3.1.24 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 5.0.15 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 5.0.16 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 6.0.3 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 7.0.0-preview.3.22175.4 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.WindowsDesktop.App 3.1.23 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 3.1.24 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 5.0.15 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 5.0.16 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 6.0.3 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 7.0.0-preview.3.22177.1 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

To install additional .NET runtimes or SDKs: https://aka.ms/dotnet-download

I will also try downloading BigDecimal code and see if the unit tests pass on my machine. Will post update when completed.

BinkersSoft commented 2 years ago

Cleaning up the .NET versions did not seem to do the trick.

When I try running the tests I get the following error:

Test project TestBigDecimal does not reference any .NET NuGet adapter. Test discovery or execution might not work for this project. It's recommended to reference NuGet test adapters in each test project in the solution.

Then it says no test is available.

BinkersSoft commented 2 years ago

Tried updating the NuGet packages and installed MSTest.TestAdapter and MSTest.TestFramework. Got a new error message saying that no tests could be found because BigDecimal-master\TestBigDecimal\bin\x64\Debug\net6.0-windows10.0.22000.0\TestBigDecimal.dll does not exist. I have confirmed the file does exist.

AdamWhiteHat commented 2 years ago

Hi, Thanks for your bug report. I looked at your screenshots, and I can confirm, subtracting 25.1 from 100.1 does NOT equal negative 25 quadrillion and some change, so something definitely is screwy. Thanks for the screenshots showing the internal values of the class, that was helpful. I have a suspicion about what might be going on. I wanted to let you know that I'm looking into the issue now. I will let you know what I find.

Regarding the project's tests (Presuming you are running the latest code from the master branch): Yeah, I just looked at these and it appears the test adapter .nuget packages called for do not support the new .net versions the project is compiling to. This is a really recent change, the project now builds 4 different assemblies that support 4 different .NET platforms (.NET 6.0, .NET 4.8 Framework, .NET core 3.1 and .NET standard), so whatever version of .NET you are using for your project, this library should be compatible. However, when I made this change, I neglected to consider the test project and what its needs were. So you've effectively identified another major bug here. I will have to find an appropriate test adapter that will work with ALL the new project target platforms. This will be the first thing I do, is get the test project working and then add your tests that are triggering the bug to the test project. I will let you know here when that is done and should be working.

One thing I did want to mention is that you should probably delete the net6.0-windows10.0.22000.0 folder out of your bin folders, or heck, just delete the bin folders all together and rebuild anew. The project no longer builds to that folder anymore, so you'll never see anything get placed in there--its not in use--but the folder gets left around because Visual Studio no longer recognizes it as an output directory for the project, and its not bold enough to delete folders it does not recognize. But yeah, I recommend removing it to avoid confusion in the future, but its not going to make the tests work. Again, Ill let you know when Ive fixed them and you should pull down the latest changes from master.

Thanks again, Adam

BinkersSoft commented 2 years ago

Hello Adam,

Thank you for the fast and detailed response. I am glad the information was helpful. Happy to test the unit tests again when you feel they are ready.

As for the subtraction problem. Something like the following works well if the exponent is the same:

if (left.Exponent == right.Exponent) { BigInteger mantissaHolder = BigInteger.Subtract(left.Mantissa, right.Mantissa); number = new BigDecimal(mantissaHolder, left.Exponent); }

For my next question, I don't expect a complete answer as it could be time consuming. But do you have any suggestions how to make the above workaround (or completely different workaround) effective when the exponents are different?

AdamWhiteHat commented 2 years ago

Okay, this is solved now. It was (thankfully) quite easy to spot once I got in there. We were implementing subtraction by reusing the addition function and negating the right hand value. Well, the Negate function was failing to pass along the exponent part of the floating point number, so it was only preserving the mantissa of the right hand value, which explains why the number got so large.

You can see the fix here on line 747: https://github.com/AdamWhiteHat/BigDecimal/pull/7/commits/c36bbc5ef2e4296982a0a23567be8e5a3cb2bab5#diff-c3ad96f4474dba8ee4f1f6b3d1c36cfa623ebb81298bc70aeb1ec2f6385dd1d4

The fix has been checked in and merged into the master branch. I have also pushed a new version to nuget, so you can just update your project's nuget version of the library https://www.nuget.org/packages/ExtendedNumerics.BigDecimal/2022.108.525.

I also fixed the test project, that should be working now. I had to find a test adapter that was compatible with the .NET framework, .NET core and .NET 6.0. I had to drop the .NET standard build of the test project (test project only), as the test adapter libraries weren't compatible with that .NET version. The BigDecimal library still has the .NET standard build, it just isn't under test, but testing the code compiled to every runtime is a bit superfluous anyway.

Last thing, just a note about the you tests as seen in the screenshots you sent. You were instantiating BigDecimals like this:

        BigDecimal high = 100.1;
        BigDecimal low = 25.1;

(You might already know all this, in which case, skip 1 paragraph down, where I tell you the best way to instantiate BigDecimals.) When you type a number with a decimal point like that in C#, and you don't give it an explicit type, I defaults to a Double, and a double being a base-2 floating point number, doesn't handle certain decimal values very well. You happened to pick two of them, and you might have noticed your high and low were being converted to the values 100.09999999999999 and 25.100000000000001, respectively. Explicitly typing them as a Double doesn't work (100.1d) or explicitly making them a Decimal (100.1m) does work in this case but no in general and suffers the same type of problems, but in base 10.

To avoid these floating point gotchas, and to ensure you get the number you intended every time, I highly encourage you to always use the static method BigDecimal.Parse to instantiate your BigDecimal classes. Its the most accurate way to instantiate a BigDecimal:

        BigDecimal high = BigDecimal.Parse( "100.1" );
        BigDecimal low = BigDecimal.Parse( "25.1" );

This also has the advantage that there is no limit to the number of decimal places you can instantiate a BigDecimal with, just remember to set BigDecimal.Precision if you need more than 5000 decimal places or turn off BigDecimal.AlwaysTruncate.

Anyways, that's all for now. I'm going to close this issue. Feel free to open another one if you still are having issues.