josdejong
bugfix/804: Added handling of complex numbers negative bases and positive infinity exponent
Implementation of following requirements in https://github.com/infusion/Complex.js/pull/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
Could give me some feedback on the custom implementation of the logic described in infusion/Complex.js#25, before I commit more work to deal with negative base cases x < -1, x == -1 and -1 < x < 0. Thanks.
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?
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
Re ComplexInfinity I think there are upto XXX different infinities we could have:
-
±∞ + 0i -
0 ± ∞i -
∞̃(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!
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.
Thanks @harrysarson and @josdejong , I agree with your comments. I'll push changes to this PR in the next few days for review.
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.
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?
@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?
I will definitely keep this in on my radar but I cannot promise I will make much progress anytime soon
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.
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.
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?
Yes of course, sorry! Thanks for the offer to at least assist getting this straightened out.