Preserve imaginary zero signs when adding real values to complex numbers#7421
Preserve imaginary zero signs when adding real values to complex numbers#7421moreal wants to merge 4 commits intoRustPython:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a private helper Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] test: cpython/Lib/test/test_builtin.py (TODO: 18) dependencies: dependent tests: (no tests depend on builtin) Legend:
|
crates/vm/src/builtins/complex.rs
Outdated
| static AS_NUMBER: PyNumberMethods = PyNumberMethods { | ||
| add: Some(|a, b, vm| PyComplex::number_op(a, b, |a, b, _vm| a + b, vm)), | ||
| add: Some(complex_add), | ||
| subtract: Some(|a, b, vm| PyComplex::number_op(a, b, |a, b, _vm| a - b, vm)), |
There was a problem hiding this comment.
how about subtract? is this okay without change?
There was a problem hiding this comment.
In subtract case, it may work because -0.0 - 0.0 (i.e., "minus-sign zero" - "plus-sign zero") is evaluated as -0.0.
fn main() {
let val: f64 = -0f64 - 0f64;
println!("{val:?}"); // -0.0
}
fn main() {
let val: f64 = -0f64 + 0f64;
println!("{val:?}"); // 0.0
}There was a problem hiding this comment.
$ python3 -c 'print(1 - (1+0j))'
-0j
$ cargo run -- -c 'print(1 - (1+0j))'
0j
There was a problem hiding this comment.
I didn't check real - complex. I'll look into it more.
There was a problem hiding this comment.
The issue was that only the real part should have been touched, but because of the conversion process to Complex64, the imaginary part was also being affected; so it was actually strange that there were no issues with subtraction. Thank you.
In CPython, these are handled by writing separate logic for each case, similar to _Py_{c,cr,rc}_xx. Accordingly, I created a helper function called complex_real_binop, like number_op, and used it to write separate logic for each case in addition and subtraction (df60617). (As for the function naming... if you have any alternatives, please let me know.)
$ cargo run -q -- -c 'print(1 - (1+0j))'
-0j
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/vm/src/builtins/complex.rs (1)
291-303: Generic type parameter ordering is inconsistent with function parameter ordering.The generic type parameters are declared as
CCF, RCF, CRF, but the function parameters are ordered ascc_op, cr_op, rc_op. This mismatch is confusing—RCF(second generic) corresponds torc_op(third parameter), whileCRF(third generic) corresponds tocr_op(second parameter).🔧 Suggested fix: reorder generic type parameters
- fn complex_real_binop<CCF, RCF, CRF, R>( + fn complex_real_binop<CCF, CRF, RCF, R>( a: &PyObject, b: &PyObject, cc_op: CCF, cr_op: CRF, rc_op: RCF, vm: &VirtualMachine, ) -> PyResult where CCF: FnOnce(Complex64, Complex64, &VirtualMachine) -> R, - CRF: FnOnce(Complex64, f64, &VirtualMachine) -> R, - RCF: FnOnce(f64, Complex64, &VirtualMachine) -> R, + CRF: FnOnce(Complex64, f64, &VirtualMachine) -> R, + RCF: FnOnce(f64, Complex64, &VirtualMachine) -> R, R: ToPyResult,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/builtins/complex.rs` around lines 291 - 303, The generic type parameter order in complex_real_binop is inconsistent with the function parameter order; reorder the type parameters from CCF, RCF, CRF to CCF, CRF, RCF so that CCF -> cc_op, CRF -> cr_op, and RCF -> rc_op (update the where-clause and all uses of these type parameters in the signature accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/vm/src/builtins/complex.rs`:
- Around line 291-303: The generic type parameter order in complex_real_binop is
inconsistent with the function parameter order; reorder the type parameters from
CCF, RCF, CRF to CCF, CRF, RCF so that CCF -> cc_op, CRF -> cr_op, and RCF ->
rc_op (update the where-clause and all uses of these type parameters in the
signature accordingly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: a2505c85-3f96-4a3a-bb84-d9d2f68c7ba1
📒 Files selected for processing (1)
crates/vm/src/builtins/complex.rs
2 participants
This pull request lets
complex + floatoperations preserve the zero sign of theimagpart.Arithmetic operations use
PyComplex::number_op, which convertsfloat(real) toComplex64to simplify logic, but it loses the zero sign. For instance, in CPython,complex(1, -0.0j) + 1becomes2-0j, andcomplex(1, -0.0j) + complex(1, 0.0j)becomes2+0j. However,PyComplex::number_opconvertsfloattocomplex, so it becomescomplex(1, 0.0j), which differs from CPython.Summary by CodeRabbit