Implement Number Protocol by qingshi163 · Pull Request #3507 · RustPython/RustPython

qingshi163 · 2021-12-20T15:41:11Z

number protocol of #3244

youknowone · 2022-04-26T08:09:57Z

@qingshi163 what have to be done to finish this PR?

youknowone · 2022-05-23T00:08:39Z

awesome!

youknowone

I have questions about cow and OnceCell part, but it looks working!
This is the way to go and we can fix other stuff later.

Do you have remaining plans to do on this PR?

youknowone · 2022-05-22T23:08:08Z

@@ -0,0 +1,242 @@
+use std::borrow::Cow;
+


Suggested change

youknowone · 2022-05-22T23:36:21Z

                };

-                try_int(&val, vm)
+                // try_int(&val, vm)


Suggested change

// try_int(&val, vm)

youknowone · 2022-05-23T10:17:27Z

+pub trait AsNumber: PyPayload {
+    #[inline]
+    #[pyslot]
+    fn slot_as_number(zelf: &PyObject, vm: &VirtualMachine) -> Cow<'static, PyNumberMethods> {


Does this slot need to return Cow? Which part prevent this to be &'static PyNumberMethods?

I think for heap object we need to use Cow::Owned witch implemented in as_number_wrapper

They are not decided by object, but decided by type. Then I think the type can hold a cloned PyNumberMethods with edited values. instead of Cow for every object when as_number is called.

I found AsMapping has the same mechanism. Then it is ok for now.

Yes, it should be stored in the type struct, but we can fix it later

qingshi163 · 2022-05-25T11:40:06Z

There is quite a lot of things not done yet, i think all the mathematic operations should be implemented via number protocol

youknowone · 2022-05-25T11:46:44Z

How about finishing this PR as current state as core implementation of AsNumber trait, and working on other types in following other PRs?
Keeping a huge PR will be a burden to maintain (to rebase everything after other merge)

youknowone · 2022-05-26T23:55:55Z

@qingshi163 are you going to make this ready for review? or do you want to do more work on it?

qingshi163 · 2022-05-27T21:13:51Z

I am still busy with it

youknowone

I think #3750 changes also be able to be adapt here.
They can be done later, but I attached a commit simplifying AsNumber interfaces.

youknowone · 2022-05-29T07:19:32Z

-                if let Some(f) = val.try_to_f64(vm)? {
-                    f
+                if cls.is(vm.ctx.types.float_type) && val.class().is(PyFloat::class(vm)) {
+                    unsafe { val.downcast_unchecked::<PyFloat>().value }


here is a regression. val object will not be reused even for same type.

youknowone · 2022-05-29T07:28:10Z

+        negative: Some(|number, vm| {
+            Self::number_downcast(number)
+                .value
+                .clone()


https://docs.rs/num/latest/num/struct.BigInt.html#impl-Neg-1

Neg is also implemented for &BigInt. This clone is redundant.

youknowone · 2022-05-29T07:30:57Z

+        absolute: Some(|number, vm| Self::number_downcast(number).value.abs().to_pyresult(vm)),
+        boolean: Some(|number, _vm| Ok(Self::number_downcast(number).value.is_zero())),
+        invert: Some(|number, vm| {
+            let value = Self::number_downcast(number).value.clone();


Not also implemented for reference.

https://docs.rs/num/latest/num/struct.BigInt.html#impl-Not-1

youknowone · 2022-05-29T07:41:37Z

+    /* Number implementations must check *both*
+    arguments for proper type and implement the necessary conversions
+    in the slot functions themselves. */


Does this mean we need to check the first argument(&PyNumber) is a correct PyNumber object?
In this case, I feel like the type will fit better in &PyObject rather than &PyNumber

The comment is from cpython but as the source shows that cpython only does the check for binary operations which could be called invertly but assume the type is correct for other functions

youknowone · 2022-05-29T08:10:45Z

+-----------+----------------------------------+-----------------------------------+----------------------------------------+
| Benchmark | benches/out/cpython38/nbody.json | benches/out/rustpython/nbody.json | benches/out/rustpython-3507/nbody.json |
+===========+==================================+===================================+========================================+
| nbody     | 103 ms                           | 1.79 sec: 17.39x slower           | 1.82 sec: 17.70x slower                |
+-----------+----------------------------------+-----------------------------------+----------------------------------------+

youknowone · 2022-05-29T21:56:02Z

at least, no regression after using references

+-----------+----------------------------------+-----------------------------------+----------------------------------------+--------------------------------------------+
| Benchmark | benches/out/cpython38/nbody.json | benches/out/rustpython/nbody.json | benches/out/rustpython-3507/nbody.json | benches/out/rustpython-3507/nbody-ref.json |
+===========+==================================+===================================+========================================+============================================+
| nbody     | 103 ms                           | 1.79 sec: 17.39x slower           | 1.82 sec: 17.70x slower                | 1.79 sec: 17.38x slower                    |
+-----------+----------------------------------+-----------------------------------+----------------------------------------+--------------------------------------------+

youknowone force-pushed the number-protocol branch from 243a130 to d5585f0 Compare April 18, 2022 15:15

qingshi163 force-pushed the number-protocol branch from d5585f0 to 55659c3 Compare May 9, 2022 19:32

qingshi163 force-pushed the number-protocol branch from 5215967 to 40a8890 Compare May 22, 2022 19:00

youknowone reviewed May 25, 2022

View reviewed changes

qingshi163 force-pushed the number-protocol branch from f1bcb2e to 4da2080 Compare May 28, 2022 19:47

youknowone mentioned this pull request May 28, 2022

Implement protocols (Abstract obejcts layer) #3244

Open

qingshi163 marked this pull request as ready for review May 29, 2022 06:53

qingshi163 requested a review from youknowone May 29, 2022 06:53

youknowone changed the title ~~[WIP] Implement Number Protocol~~ Implement Number Protocol May 29, 2022

youknowone requested changes May 29, 2022

View reviewed changes

youknowone force-pushed the number-protocol branch from 06a0d1c to 1fd5d0a Compare May 29, 2022 21:55

PyNumber protocol

beeb449

youknowone force-pushed the number-protocol branch 2 times, most recently from 1b9b663 to 7a61d07 Compare May 29, 2022 22:34

qingshi163 added 4 commits May 30, 2022 07:36

Number protocol for PyInt

20632ed

deprecated warnings for int

59cedd2

impl number protocol float

3e6e348

fix vm.to_index now use number protocol

1ed18c0

youknowone force-pushed the number-protocol branch from 7a61d07 to 0c9d9a5 Compare May 29, 2022 22:37

qingshi163 and others added 2 commits May 30, 2022 07:38

Simplify AsNumber trait

ea95777

Use static ref

09fc676

youknowone force-pushed the number-protocol branch from 0c9d9a5 to 09fc676 Compare May 29, 2022 22:38

youknowone approved these changes May 29, 2022

View reviewed changes

Comment thread vm/src/protocol/number.rs Outdated

Comment thread vm/src/protocol/number.rs Outdated

youknowone merged commit c40bc62 into RustPython:main May 29, 2022

fanninpm mentioned this pull request Jul 3, 2022

Implement Number protocol for PyComplex #3838

Merged

qingshi163 deleted the number-protocol branch July 3, 2022 19:19

fanninpm mentioned this pull request Jul 29, 2022

Unable to install RustPython from source with cargo install. #3982

Closed

Conversation

qingshi163 commented Dec 20, 2021 • edited by youknowone Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

youknowone commented Apr 26, 2022

Uh oh!

youknowone commented May 23, 2022

Uh oh!

youknowone left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qingshi163 commented May 25, 2022

Uh oh!

youknowone commented May 25, 2022

Uh oh!

youknowone commented May 26, 2022

Uh oh!

qingshi163 commented May 27, 2022

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

youknowone commented May 29, 2022

Uh oh!

youknowone commented May 29, 2022

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

qingshi163 commented Dec 20, 2021 •

edited by youknowone

Loading

youknowone left a comment •

edited

Loading