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

bugfix/804: Added handling of complex numbers negative bases and positive infinity exponent #2097

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

georgemarklow
Copy link

Implementation of following requirements in rawify/Complex.js#25:

  • z ^ Infinity === NaN
  • Infinity ^ z === Infinity if Im(z) === 0 and Re(z) > 0
  • Infinity ^ z === 0 if Re(z) < 0
  • Infinity ^ 0 === 1
  • Infinity ^ z === NaN otherwise

@georgemarklow
Copy link
Author

Could give me some feedback on the custom implementation of the logic described in rawify/Complex.js#25, before I commit more work to deal with negative base cases x < -1, x == -1 and -1 < x < 0. Thanks.

@josdejong
Copy link
Owner

Looks good and well tested @georgemarklow , thanks 👍

I remember we had some discussions long time ago on whether we should return number Infinity, or complex Infinity, it may be this discussion: #1277. Do you have any thoughts in that regard George?

@georgemarklow
Copy link
Author

Thanks @josdejong for your feedback.

I agree with what you said at the bottom of that thread that ComplexInfinity covers a number of edge cases, and it's also consistent with Wolfram's view and therefore ideal for users consuming mathjs that have come from that background. If you agree - I'll create a ComplexInfinity type and I'll make the changes needed.

Let me know what you feel is the best choice.

Thanks!
George

@harrysarson
Copy link
Collaborator

Re ComplexInfinity I think there are upto XXX different infinities we could have:

  1. ±∞ + 0i
  2. 0 ± ∞i
  3. ∞̃ (ComplexInfinity: infintite magnitude and unknown/unspecified/undefined argument)

Currently this PR only covers postive real infinity --- so the first half of (1.). The change looks good to me but it might be nice to extend it to correctly handle the other infinities. Probably the best way to do that is to add an isFinite function to mathjs (or update the implementation if there is already one) rather than checking x.re == Infininity.

Thanks for picking this up @georgemarklow it would be really cool to see this fixed finally!

@josdejong
Copy link
Owner

I agree with what you said at the bottom of that thread that ComplexInfinity covers a number of edge cases, and it's also consistent with Wolfram's view and therefore ideal for users consuming mathjs that have come from that background. If you agree - I'll create a ComplexInfinity type and I'll make the changes needed.

Yes it makes sense to me 👍 . I guess Harry is right though that this may open up new edge cases that we need to be aware of.

Probably the best way to do that is to add an isFinite

Good idea! Or make an isInfinite ;) . Similar to isNaN.

@georgemarklow
Copy link
Author

Thanks @harrysarson and @josdejong , I agree with your comments. I'll push changes to this PR in the next few days for review.

@georgemarklow
Copy link
Author

Hi - I've checked in my progress so far. I'll come back to this PR later in the week to resolve some commented unit tests and check test coverage. In the meantime, if there's anything in the PR you would like changed at this stage, just let me know. Thanks.

@harrysarson
Copy link
Collaborator

Hey sorry for the slowness, I have been thinking about this issue though. I think this is the behaviour we want. Does this agree with your thinking so far @georgemarklow?

@josdejong
Copy link
Owner

@harrysarson George let me know that unfortunately he isn't able to work on this any further. Is this something you like to pick up instead?

@harrysarson
Copy link
Collaborator

I will definitely keep this in on my radar but I cannot promise I will make much progress anytime soon

@josdejong
Copy link
Owner

👍 take it easy

@gwhitney
Copy link
Collaborator

I must admit I am confused about the current handling of complex infinities/NaNs both in mathjs and complex.js, as there seem to be lots of open issues and PRs on this topic in both projects, but I'd rate this as fairly high priority to iron out in some consistent way and implement and move on from... if that requires some effort, I'm happy to help but can't guarantee I will be able to do this entirely on my own.

@josdejong
Copy link
Owner

Thanks Glen! Yes this could involve quite some edge cases. We can possibly address issues in multiple small PR's that each improve some part to keep it manageable.

@gwhitney
Copy link
Collaborator

I have so many assigned to me now, and I tried to make it clear above that I do not see myself as being able to handle this one on my own, so is it OK if I unassign myself?

@josdejong
Copy link
Owner

Yes of course, sorry! Thanks for the offer to at least assist getting this straightened out.

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.

power returns invalid complex number for negative base and Infinity as exponent
4 participants