Skip to content
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

Add conversion from Python int to either long or BigInt properly #1966

Closed
cmpute opened this issue Nov 5, 2021 · 6 comments
Closed

Add conversion from Python int to either long or BigInt properly #1966

cmpute opened this issue Nov 5, 2021 · 6 comments

Comments

@cmpute
Copy link
Contributor

cmpute commented Nov 5, 2021

Sometimes it's convenient to have a conversion to proper type in rust, i.e. if the Python int is larger than 2^64, then convert to BigInt, if smaller, then convert to native i64. This could reduce some overhead if you want to be able to deal with big number and with small number efficiently at the same time.

The following code is what I figured out and it's working, but it might be better if it's integrated into the support for num_bigint

use pyo3::{ffi, AsPyPointer};
use pyo3::types::{PyLong, PyType};
use std::os::raw::c_int;

enum IntTypes {
    Small(i64),
    Big(BigInt),
}

fn parse_int(ob: &PyAny) -> PyResult<IntTypes> {
    let py = ob.py();
    unsafe {
        let num: Py<PyLong> = Py::from_owned_ptr_or_err(py, ffi::PyNumber_Index(ob.as_ptr()))?;
        let mut overflow: c_int = 0;
        let v = ffi::PyLong_AsLongLongAndOverflow(num.as_ptr(), &mut overflow);
        if v == -1 && PyErr::occurred(py) {
            Err(PyErr::fetch(py))
        } else if overflow != 0 {
            let num: BigInt = ob.extract()?;
            Ok(IntTypes::Big(num))
        } else {
            Ok(IntTypes::Small(v))
        }
    }
    // then use ToPyObject
}
@davidhewitt
Copy link
Member

Thank you for the proposal. I would recommend instead solving this using #[derive(FromPyObject)] - this will generate an implementation which will first try to extract a Small, and if that fails, try a Big.

#[derive(FromPyObject)]
enum IntTypes {
    Small(i64),
    Big(BigInt),
}

As the above is straightfoward (although not super well documented, sorry), I don't think there's need to add a more specific function for this to PyO3.

@cmpute
Copy link
Contributor Author

cmpute commented Nov 5, 2021

@davidhewitt Thanks for the information! I indeed found that the documentation for enums is quite lacky. This solution is quite neat!

However I think using my code snippet above will reduce the overhead for conversion (without depending on Python throwing errors), although it might not be critical in most cases.

@davidhewitt
Copy link
Member

I agree that your snippet will be more efficient, however I think a small enough optimisation that at this point I don't feel a strong need to add this to PyO3. If this proves to be a popular request, I'm happy to change my mind later.

FYI - If you tweaked your parse_int function to be an impl FromPyObject for IntTypes then you'll be able to use IntTypes directly in #[pyfunction] and #[pymethods] arguments.

@cmpute
Copy link
Contributor Author

cmpute commented Dec 21, 2021

Just a quick note, I found about 0.2 s improvement on my laptop with 10000 evaluations of a function taking IntTypes as input.

@davidhewitt
Copy link
Member

After #2068 this performance gap should hopefully be a lot smaller 🤞

@cmpute
Copy link
Contributor Author

cmpute commented Dec 22, 2021

Cool! Thanks for the efforts!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants