Skip to content

Conversation

@circle-gon
Copy link

Currently, raising negative powers to a fraction is broken in b_e. This PR addresses this issue.

For example, new Decimal(-8).cbrt() and new Decimal(-8).pow(1/3) both return NaN instead of -2.

This PR would fix this behavior, so both of the examples will return -2 instead of NaN.

New tests testing this behavior have been added.

@MathCookie17
Copy link
Collaborator

What about Decimal.pow(-8, 2/3)? That should return positive 4, right? It still returns NaN right now.

@HexaVault
Copy link

What about Decimal.pow(-8, 2/3)? That should return positive 4, right? It still returns NaN right now.

.pow(n, 2/3) atleast has a workaround with this (.pow(n, 1/3).pow(2)) while the current root implementation gives us no workaround

@MathCookie17
Copy link
Collaborator

.pow(n, 2/3) atleast has a workaround with this (.pow(n, 1/3).pow(2)) while the current root implementation gives us no workaround

Yeah, but I don't see much reason to merge a fix like this if it's not going to go the full distance.

@HexaVault
Copy link

.pow(n, 2/3) atleast has a workaround with this (.pow(n, 1/3).pow(2)) while the current root implementation gives us no workaround

Yeah, but I don't see much reason to merge a fix like this if it's not going to go the full distance.

well then why not suggest a fix

@MathCookie17
Copy link
Collaborator

MathCookie17 commented Sep 24, 2024

well then why not suggest a fix

Because the fix I can think of would involve taking an arbitrary Decimal and approximating it as a rational number (using continued fractions or something) in order to figure out what its denominator should be, which seems a little ridiculous to do for something as simple as pow.

@HexaVault
Copy link

well then why not suggest a fix

Because the fix I can think of would involve taking an arbitrary Decimal and approximating it as a rational number (using continued fractions or something) in order to figure out what its denominator should be, which seems a little ridiculous to do for something as simple as pow.

so, if im understanding this correctly:

You want an implementation that likely would do the same fix you have suggested, which you think is rediculous to do.

Again, if you think any and all implementations would be unreasonable, then why ask the question about not going all the way to begin with. n^(a/b) has a very obvious n^(1/b)^(a) workaround and this edgecase only even occurs if n is negative and b is odd, so its rare enough to not matter. On the other hand, roots occur fairly frequently (cbrt and sqrt literally have their own functions for this reason) and so something like this requires a fix.

Obviously n^(1/b) has the workaround abs(n)^(1/b) × sign(n) but this isnt immediately obvious, and if the library has a simple fix to an issue it should provide said fix rather than force a very ugly workaround everytime something that is rooted could be negative.

@MathCookie17
Copy link
Collaborator

MathCookie17 commented Sep 26, 2024

so, if im understanding this correctly:

You want an implementation that likely would do the same fix you have suggested, which you think is rediculous to do.

Again, if you think any and all implementations would be unreasonable, then why ask the question about not going all the way to begin with. n^(a/b) has a very obvious n^(1/b)^(a) workaround and this edgecase only even occurs if n is negative and b is odd, so its rare enough to not matter. On the other hand, roots occur fairly frequently (cbrt and sqrt literally have their own functions for this reason) and so something like this requires a fix.

Obviously n^(1/b) has the workaround abs(n)^(1/b) × sign(n) but this isnt immediately obvious, and if the library has a simple fix to an issue it should provide said fix rather than force a very ugly workaround everytime something that is rooted could be negative.

I was hoping there might be a solution that's easier than the one I came up with (but I don't know what that might be), which is why I didn't suggest my solution until prompted. I feel like having it so non-whole powers of negatives are always NaN is more consistent than "unit fraction powers are defined, but not other rational numbers with odd denominator", although perhaps cbrt specifically should be changed to account for the negative case since it's a separate function.

@HexaVault
Copy link

so, if im understanding this correctly:

You want an implementation that likely would do the same fix you have suggested, which you think is rediculous to do.

Again, if you think any and all implementations would be unreasonable, then why ask the question about not going all the way to begin with. n^(a/b) has a very obvious n^(1/b)^(a) workaround and this edgecase only even occurs if n is negative and b is odd, so its rare enough to not matter. On the other hand, roots occur fairly frequently (cbrt and sqrt literally have their own functions for this reason) and so something like this requires a fix.

Obviously n^(1/b) has the workaround abs(n)^(1/b) × sign(n) but this isnt immediately obvious, and if the library has a simple fix to an issue it should provide said fix rather than force a very ugly workaround everytime something that is rooted could be negative.

I was hoping there might be a solution that's easier than the one I came up with (but I don't know what that might be), which is why I didn't suggest my solution until prompted. I feel like having it so non-whole powers of negatives are always NaN is more consistent than "unit fraction powers are defined, but not other rational numbers with odd denominator", although perhaps cbrt specifically should be changed to account for the negative case since it's a separate function.

As I said, unit fractions make sense because of the root function, which uses unit fractions into powers, hence the reason this pr exists

@circle-gon
Copy link
Author

Maybe this PR should just patch the .cbrt() function and leave the .pow() function alone? It also aligns with javascript's Math.cbrt and Math.pow behavior, although I'm not sure if that matters.

MathCookie17 added a commit that referenced this pull request Oct 7, 2024
Also fixed #172 in the process
@MathCookie17
Copy link
Collaborator

cbrt and root have been changed to allow negative numbers (provided the root's degree is an odd integer in the latter's case). pow is unchanged. Since I didn't implement the pow change originally suggested, I'll leave it up to you whether to close this pull request rather than closing it myself.

@circle-gon
Copy link
Author

Seems to be good enough.

@circle-gon circle-gon closed this Oct 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants