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

BUG: BigDecimal.Pow(5d, 0.5d) no longer works #45

Closed
MileyHollenberg opened this issue May 6, 2024 · 6 comments · Fixed by nissl-lab/npoi#1350
Closed

BUG: BigDecimal.Pow(5d, 0.5d) no longer works #45

MileyHollenberg opened this issue May 6, 2024 · 6 comments · Fixed by nissl-lab/npoi#1350
Assignees
Labels

Comments

@MileyHollenberg
Copy link

Description
In the Readme there's an example which uses BigDecimal.Pow(5d, 0.5d) but this is no longer working as the Pow method no longer accepts a double value, only a BigInteger.

Error/Exception Message
Compile time error

Expected Behavior
Having the ability to raise a BigDecimal value to the power of a decimal based value

Actual Behavior
You can only raise BigDecimal's to the power of a integer value

Steps To Reproduce
Simply add BigDecimal.Pow(5d, 0.5d) somewhere in your code and see that it won't compile

Screenshots
N/A

Additional context/Comments/Notes/Rants
Seems it got removed in this commit

@AdamWhiteHat
Copy link
Owner

Sorry about that. I certainly didn't intent to remove that method. I review every file and line before I commit changes, I must have not been paying very close attention.

Thank you for bringing this to my attention.

I have build and released a new NuGet package, version 2025.1001.2.129 that contains this fix.

@MileyHollenberg
Copy link
Author

MileyHollenberg commented May 8, 2024

Thanks, this looks to be back again :). Would it maybe be possible to also get a version that accepts a BigDecimal as the first argument? I've tried to create one myself but this seems to be quite difficult (the LogN and Exp functions frequently freeze up due to how much work they would need to do)

For clarity, I was trying to get it to work like this (came from ChatGPT as I'm not the strongest with this kind of math in general so it might be flawed from the start already)

  public static BigDecimal Pow(BigDecimal @base, BigDecimal exponent)
  {
      // Convert exponent to natural logarithm
      BigDecimal logarithm = BigDecimal.Log(@base);
      
      // Multiply logarithm by the decimal exponent
      BigDecimal resultLog = BigDecimal.Multiply(logarithm, exponent);
      
      // Convert back to original scale
      BigDecimal result = BigDecimal.Exp(resultLog);
      
      return result;
  }

@AdamWhiteHat
Copy link
Owner

AdamWhiteHat commented May 18, 2024

Would it maybe be possible to also get a version that accepts a BigDecimal as the first argument?

For clarity, I was trying to get it to work like this (came from ChatGPT as I'm not the strongest with this kind of math in general so it might be flawed from the start already)

  public static BigDecimal Pow(BigDecimal @base, BigDecimal exponent)
  {
     ...

This method is essentially correct. The only thing it got wrong is there is no BigDecimal.Log() method; Use BigDecimal.Ln() instead.

I didn't think it was correct at first, so I implemented it using the same principal, then compared the methods. This is what I made:

public static BigDecimal Pow(BigDecimal @base, BigDecimal exponent, int precision)
{
	//  b^p = x^(p * log_x(b)) = e^(p*ln(b))
	// because log_x(b^p) = p * log_x(b)

	BigDecimal ln_b = BigDecimal.Ln(@base, precision);
	return BigDecimal.Exp(exponent * ln_b, precision);
}

But you are correct about behavior of LogN and Exp: Because these are implemented using the Taylor series, the asymptotic complexity depends almost entirely on its input; some values take hundreds of iterations to converge, others might take just 3.

And in the above code, we are calling both Ln and Exp, increasing the chances that one of them will not like its input value and bog down.

So for this reason, it would probably be better if I just wrote an exponentiation function directly. I have made a start on this. I don't anticipate running into any difficulties.

In the interim, use the above method for now, I guess.

Repository owner deleted a comment from adamatpinbn Jul 26, 2024
@AdamWhiteHat
Copy link
Owner

@MileyHollenberg

I went ahead and checked that version of the Pow operation (the one that uses logs) into the BigDecimal code on the main branch. If you pull the latest version of the code, or grab the NuGet package version 2025.1002.0.190 or higher, you should see this method as one of the available BigDecimal.Pow overloads.

I wasn't able to implement a 'exponentiation function directly', as at some point it has to rely on either nth roots or logarithms to get the job done; it cant be exponentiation all the way down. And the current nth root implementation, in turn, relies on exponentiation. Preferring to avoid an infinite recursive loop, I am left with only one option: logarithms.

The above Pow method is slow because the current log natural method is slow; it uses a guess and refine method, which would be fine, except the Exp method that it uses each iteration is implemented using a Taylor series. As fast as the Taylor series is, calculating one to high precision every iteration for several iterations is very inefficient and is where all the time is being spent.

I plan to replace this inefficient log natural method with one that calculates the natural log using a Taylor series directly. Initial testing shows this approach to be between 3 to 28 times faster, depending on how much precision you are requesting. The only reason its not published in the library now is the method is hitting a break condition and returning a result that is only accurate to approx. half of the requested precision, and I'm not sure why. A crude hack would be to double the requested precision parameter before passing it along to the Taylor series method, but I'd rather understand what is going on and take a more principled approach. Perhaps if my level of exasperation becomes great enough you'll see something like that.

But I was looking to close this issue, if you're satisfied. Closed or not, I will drop a comment on this thread when I have pushed an update that improves the speed/performance of this Pow(BigDecimal, BigDecimal) method overload, so watch this space.

@AdamWhiteHat
Copy link
Owner

@MileyHollenberg I replace the Ln (Natural Logarithm) method with a much more performant version like I said I would, and this should, in turn, improve the speed of the Pow(BigDecimal, BigDecimal) method, which relies on it.

This improvement is available in BigDecimal's NuGet package version 2025.1003.0.225 and greater, or just pull the latest changes from the main branch.

@MileyHollenberg
Copy link
Author

Sorry for the radio silence on my part, thanks for fixing this!

I did however switch back to just using a regular double in my game as there were some inconsistencies with the math (values that should only be able to go up were sometimes going back down for no real reason) plus a double can easily manage my use-case for a very long time so it's more performant for my usecase to go with that instead.

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

Successfully merging a pull request may close this issue.

2 participants