-
Notifications
You must be signed in to change notification settings - Fork 286
Complex improment #817
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
Complex improment #817
Conversation
1e14ba5 to
ff0000b
Compare
src/runtime/classobj.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like CPython doesn't have their equivalent of this -- are you sure we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see test_complex, line 228, there has class class OS, seems it could only work if I add __complex__ to classobj.cpp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, most oldstyle-class attribute lookups get automatically proxied. It's just for the attributes that have capi slots where we need to provide a wrapper implementation, and I don't think there's one for __complex__. Could you try the test without this and see if it works?
|
Hmm ok so this is a lot of code that I don't have any understanding of, and a bunch of it is structured a little bit differently than CPython does it. I think we have two main options:
If we were starting from scratch I'd advocate for the second option, but since you've gone pretty far down the first path I think it'd be fine to continue with it. I still think it'll be less total work to just copy CPython's implementation (for example, we pass different format arguments for complexRepr... I have no idea what the impact there is), but I'll leave it up to you. |
ff0000b to
9c193a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If type(y) is complex, type(x) is xcomplex:
type(pow(y, x)) and type(x ** y)
===> xcomplex.
type(y ** x)
===> complex
It should be xcomplex.
Could anyone give some helps? FYI, originally I use my own implementation of complexPow, the error occured and could not figure out why. Then I switch to the pure CPython implementation. Same problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bug / missing feature in our binop handling; usually the rule is you try lhs.__pow__(rhs) and then try rhs.__rpow__(lhs), except if rhs is a subclass of lhs, and then you reverse the order. We don't support that reverse-order case, and I don't think it's specific to complex or pow. It'd be great if you wanted to look into it (we would want to rewrite it, which could be a good intro to that stuff), but I think for this PR it's fine to comment out the check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will try to look at the reverse-order binop. But recently I am busy(one or two weeks). But I will continue to improve the compatibility in my spare time.
a14cdd3 to
0ada720
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is libpypa bug, Already solved in local repository, and create a PR vinzenz/libpypa#47. Maybe libpypa will use another solution, but it will be solved at last. When it done, re-enable it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add this as a note in the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And same thing for the other changes to this file -- can we have Pyston change comments with them explaining why it differs from CPython.
0ada720 to
087bfe7
Compare
087bfe7 to
3159688
Compare
|
awesome :) |
Change:
assert. Let the behavior consistent with CPython.complexNew. Let it could accept keyword arguments, such ascomplex(real=xx, imag=yy). And also acceptcomlex,long,str,unicodeasrealorimagargument.But could not handle custom class with__complex__yet.complexFmtbased on CPython implementation. Let it could remove empty decimal point.complexrelated changes.closes #779