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

Fix BigMath::exp for negative immediate types, add tests for these cases #332

Closed
wants to merge 1 commit into from

Conversation

@GarthSnyder
Copy link

GarthSnyder commented Jun 18, 2013

Please note: I'm not 100% sure that this fix is the correct fix, so this patch requires careful scrutiny by someone familiar with the BigDecimal implementation. However, the test cases I added do show a clear problem with BigMath.exp of immediate types with values < 0 and may be useful to integrate regardless of whether the fix goes in.

Problem: BigMath.exp(-x, n) == BigMath.exp(x, n) whenever x is an immediate type. For example, BigMath.exp(-1, 20) is 2.718. The correct value is 0.3679 (that is, 1/2.718)

Mechanism: It appears that the VALUE x passed into BigMath_s_exp is promoted to a Real vx. At bigdecimal.c:2742, the code flips the sign bit on negative values as part of the implementation of the identity that E * -x == 1 / (E ** x). Later at line 2774, the sign is rechecked and the reciprocal of the result is returned if the original argument was negative.

However, in the calculation loop at line 2765, it's the original VALUE x that's used in the iteration instead of vx->obj. This works OK for BigDecimals since VPSetSign reaches though vx into the original x. (That is itself a separate problem, since it results in the argument being unexpectedly modified; I will submit a separate issue for that.) But immediate types still have their original (negative) values.

Fix: I suspect that it's just ToValue(vx) that's intended here instead of x.

@GarthSnyder

This comment has been minimized.

Copy link
Author

GarthSnyder commented Jun 18, 2013

This issue also occurs in 1.9.3 and probably 1.8 as well.

@hsbt

This comment has been minimized.

Copy link
Member

hsbt commented Jun 25, 2013

@mrkn Could you review this pull request?

@mrkn

This comment has been minimized.

Copy link
Member

mrkn commented Jun 25, 2013

I'll check it tonight.

@evanphx evanphx closed this in 448c66c Jun 25, 2013
evanphx pushed a commit that referenced this pull request Jul 1, 2013
	* ext/bigdecimal/bigdecimal.c (BigMath_s_exp): Fix for the cases when
	  the argument x is not a BigDecimal.
	  This change is based on the patch made by Garth Snyder.
	  [Fix GH-332] #332
	  This change is based on the patch made by Heesob Park and Garth Snyder.
	  [Bug #6862] [ruby-core:47145]


git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/branches/ruby_2_0_0@41733 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
evanphx pushed a commit that referenced this pull request Jul 11, 2013
	* ext/bigdecimal/bigdecimal.c (BigMath_s_exp): Fix for the cases when
	  the argument x is not a BigDecimal.
	  This change is based on the patch made by Garth Snyder.
	  [Fix GH-332] #332
	  This change is based on the patch made by Heesob Park and Garth Snyder.
	  [Bug #6862] [ruby-core:47145]


git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/branches/ruby_1_9_3@41909 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
tenderlove pushed a commit to tenderlove/ruby that referenced this pull request Jan 24, 2014
  the argument x is not a BigDecimal.
  This change is based on the patch made by Garth Snyder.
  [Fix rubyGH-332] ruby#332

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@41623 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
mrkn added a commit to ruby/bigdecimal that referenced this pull request Dec 25, 2015
  the argument x is not a BigDecimal.
  This change is based on the patch made by Garth Snyder.
  [Fix GH-332] ruby/ruby#332

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@41623 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.