-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
base: develop
Are you sure you want to change the base?
Conversation
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. |
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! |
Re
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 Thanks for picking this up @georgemarklow it would be really cool to see this fixed finally! |
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.
Good idea! Or make an |
Thanks @harrysarson and @josdejong , I agree with your comments. I'll push changes to this PR in the next few days for review. |
…ity or -Infinity. Further unit tests required.
…://github.com/georgemarklow/mathjs into bugfix/804-pow-returns-invalid-complex-number
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 |
👍 take it easy |
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. |
Implementation of following requirements in rawify/Complex.js#25: