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

feat: modify VectorInit to handle numpy multi dimensional arrays #28

Closed
wants to merge 20 commits into from

Conversation

Khushiyant
Copy link

No description provided.

@wlav
Copy link
Owner

wlav commented Jun 25, 2024

Two points: first, don't change comment indentation (I know the style is a bit unusual, but the reason for it is that it makes super-easy to read either the code or the comments, instead of having both in the same flow) and don't remove white lines if it changes the style.

Second, as discussed, the NumPy folks designed the "new-style" buffer protocol: https://docs.python.org/3/c-api/buffer.html . There are examples of its use in Utilities.cxx and LowLevelViews.cxx. The former is more convoluted b/c it also supports the "old-style" protocol (really old at this point); the latter shows how multi-dim arrays are handled.

As for the use of creating the std::vector on the Python side, I think that's fine. In LowLevelViews.cxx, the code is a single template that is instantiated for every builtin type. If performance becomes an issue, or if any of this code has to be lowered for Numba, then that's still an option. Can even pre-instantiate the common types (double, float, int) and let the rest go through Python, to get the best of both worlds.

@Khushiyant Khushiyant marked this pull request as ready for review July 30, 2024 09:20
@Khushiyant Khushiyant changed the title chore: modify VectorInit to handle numpy multi dimensional arrays feat: modify VectorInit to handle numpy multi dimensional arrays Jul 30, 2024
@Khushiyant Khushiyant force-pushed the numpy-dim branch 2 times, most recently from e249453 to 2d5f981 Compare August 13, 2024 14:02
@@ -52,6 +52,24 @@ bool HasAttrDirect(PyObject* pyclass, PyObject* pyname, bool mustBeCPyCppyy = fa
return false;
}

template <typename T>
T Get_IndexValue(Py_buffer *view, int i)
Copy link

@vgvassilev vgvassilev Aug 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
T Get_IndexValue(Py_buffer *view, int i)
bool Get_IndexValue(Py_buffer *view, T* value)

for (Py_ssize_t i = 0; i < fillsz; i++)
{
int val = Get_IndexValue<int>(view, i);
PyObject *item = PyLong_FromLong(val);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mapping of view->format to typename (e.g. 'i' -> 'int'; see low-level view type_traits).

TConverter* conv = CreateConverter(type_returned);
PyObject* item = conv->FromMemory(view->buf + i*view->itemsize);
if (conv->HasState()) delete conv;

@Khushiyant
Copy link
Author

Implemented this PR logic in cppyy (frontend) - cppyy#255 instead

@Khushiyant Khushiyant closed this Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants