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

Safe wrapper for NPyIter #129

Closed
PTNobel opened this issue Jun 12, 2020 · 12 comments
Closed

Safe wrapper for NPyIter #129

PTNobel opened this issue Jun 12, 2020 · 12 comments

Comments

@PTNobel
Copy link
Contributor

PTNobel commented Jun 12, 2020

I'm evaluating using rust-numpy for a project that will require extensive use of NPyIter as I have to re-implement a number of ufuncs and need access to the broadcasting implementation.

I'm currently using np.broadcast in Python code to implement this.

Would there be any interest in making a safe wrapper for NPyIter if I built one?

Does anyone have any advice or similar wrappers I could reference? This is my first experience with writing unsafe rust code.

@kngwyu
Copy link
Member

kngwyu commented Jun 15, 2020

Thank you!
It's interesting to support NpyIter and I'm willing to give advises 🙂

Does anyone have any advice or similar wrappers I could reference?

Since NpyIter is not a Python type, we have no example and it can be difficult 🤔
Maybe we have to wrap it with a lifetime like NpyIter<'py> but I'm not sure how to do so.

@PTNobel
Copy link
Contributor Author

PTNobel commented Jun 15, 2020

Thanks kngwyu! I'll let you know when I get started on this and I'll post questions here?

As for the lack of examples, well I guess that just makes my job more fun.

Maybe we have to wrap it with a lifetime like NpyIter<'py> but I'm not sure how to do so.

Is your idea regarding lifetimes that a RustNpyIter object needs to have a litetime longer then the lifetime of any array used to create it?

@kngwyu
Copy link
Member

kngwyu commented Jun 16, 2020

Is your idea regarding lifetimes that a RustNpyIter object needs to have a litetime longer then the lifetime of any array used to create it?

Yes, maybe our NpyIter looks like:

struct NpyIter<'py> {
    ptr: NonNull<npyffi::NpyIter>,
    py: Python<'py>,
}

@kngwyu
Copy link
Member

kngwyu commented Jun 16, 2020

Note that npyffi::NpyIter is an opaque type so we can only access its pointer.

@PTNobel
Copy link
Contributor Author

PTNobel commented Jun 17, 2020

Any advice on ergonomicly handling the following parameters of NpyIter_New and NpyIter_MultiNew?

npy_uint32 flags: this is just a bunch of constants you can or together to change the behaviour of NpyIter.

npy_intp nop, PyArrayObject** op,: This is for MultiNew; I'm not sure how to best handle this, one obvious approach is to take a Vector of PyArrays or something along those lines, but that doesn't feel very clean. Any sense of what's standard/ergonomic for Rust?

@PTNobel
Copy link
Contributor Author

PTNobel commented Jun 17, 2020

Also entries in op can be NULL which causes the MultiIndex to allocate a new array.

@PTNobel
Copy link
Contributor Author

PTNobel commented Jun 17, 2020

The rust wrapper also needs to keep track of a seperate entry called the op_flags which are assoicated with each passed in op because that determines if we can return a &mut dtype or a &dtype. Also, is there a safe way to handle WRITEONLY pointers? Should we just initialize any value to 0 before returning it to avoid reading uninitialized memory in safe rust?

@PTNobel
Copy link
Contributor Author

PTNobel commented Jun 17, 2020

I'm thinking something inspired by OpenOptions from std::fs is probably best, as it should be easy and ergonimic to set the flags as desired. Not quite sure how best to specify the different options for the multiiter since some of them have to be seperated from flags and into op_flags.

I could use a different type for the MultiIter constructor and perhaps do something like constructor.set_flag(FlagEnum::FlagName).add_array(myfirst_pyarray).set_flag(OpFlagEnum::FlagName, true)..add_array(mysecond_pyarray).set_flag(OpFlagEnum::FlagName, true).set_flag(OpFlagEnum::AnotherFlagName, true). Note that constructor` and the return value of add_array would have to be different, but that probably makes sense anyways for this.

I also don't think there's a huge need for MultiIter and the 1D Iter to share a wrapper, as they do fundamentally behave differently.

@PTNobel
Copy link
Contributor Author

PTNobel commented Jun 17, 2020

I'm also thinking I should avoid NumPy setting any Exceptions, and instead have it return error strings that can be passed up to the caller so that the caller can choose whether to raise an exception.

@kngwyu
Copy link
Member

kngwyu commented Jun 17, 2020

something inspired by OpenOptions from std::fs

Maybe it is what is often called the builder pattern.

I'm also thinking I should avoid NumPy setting any Exceptions, and instead have it return error strings that can be passed up to the caller so that the caller can choose whether to raise an exception.

PyO3 itself can handle exceptions. You don't have to worry about it.

Also, is there a safe way to handle WRITEONLY pointers?

You can use MaybeUninit but it can be difficult.
For now, I recommend you to build the easiest one(maybe READONLY?) first.
Such safety problems are often hard so maintainers address them.

@messense
Copy link
Member

I think we can close this issue?

@kngwyu
Copy link
Member

kngwyu commented Jul 12, 2021

👍🏼

@kngwyu kngwyu closed this as completed Jul 12, 2021
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

3 participants