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 py::array::size() function? #34

Closed
adler-j opened this issue Dec 15, 2015 · 6 comments
Closed

Add py::array::size() function? #34

adler-j opened this issue Dec 15, 2015 · 6 comments

Comments

@adler-j
Copy link
Contributor

adler-j commented Dec 15, 2015

Currently if I want to access a parameter such as array length, the best method to my knowledge is

size_t size = target.request().count;

There are two problems with this. First, the corresponding python name is size, so renaming count -> size would be nice.

Further, wouldnt It be nice to have access to this as a free function?

size_t size = target.size()
@wjakob
Copy link
Member

wjakob commented Dec 15, 2015

Not sure I agree. Having both an 'itemsize' and 'size' field is highly confusing -- calling it 'count' disambiguates which one is a size in bytes, and which one is the element count.

The problem with directly exposing size() is that it's an expensive operation. You have to request a buffer view via PyObject_GetBuffer and release it right afterwards (these are heavy operations). It's better to request it only once, and to encourage this kind of usage via the API.

@adler-j
Copy link
Contributor Author

adler-j commented Dec 15, 2015

Not sure I agree. Having both an 'itemsize' and 'size' field is highly confusing -- calling it 'count' disambiguates which one is a size in bytes, and which one is the element count.

I would agree if this was a "free standing" library and we were free to select new names, but (imo) numpy compatibility is a very strong feature here. In addition size is the name used in all stl containers in c++, while count is used for counting something, such as in std::count.

The problem with directly exposing size() is that it's an expensive operation. You have to request a buffer view via PyObject_GetBuffer and release it right afterwards (these are heavy operations). It's better to request it only once, and to encourage this kind of usage via the API.

If that is the case, I agree, but Isn't PyArray_SIZE decently fast? At worst you incur the same overhead that numpy would, which for many users is ok.

@wjakob
Copy link
Member

wjakob commented Dec 15, 2015

That's a good point -- if it's possible to replicate the behavior of PyArray_SIZE without depending numpy headers, then I'm open to adding a function to pybind11::array. However, this would not work for the bare bones pybind11::buffer interface.

@wjakob wjakob changed the title Feedback on py::array.request() Add py::array::size() function? Dec 15, 2015
@adler-j
Copy link
Contributor Author

adler-j commented Dec 15, 2015

Regarding the rename, the issue started more ambitious: allow access of each buffer member one-by-one instead of through the buffer_info protocol.

@wjakob
Copy link
Member

wjakob commented Dec 15, 2015

The issue I see here is that it's impossible to provide these fast access operations to structural data for non-NumPy plain buffer objects.

On the other hand, the bindings of pybind11::array should be compatible with pybind11::buffer. Not necessarily impossible to do, but something to take into account.

@adler-j
Copy link
Contributor Author

adler-j commented Dec 22, 2015

I had a bug that seems to be related to this, when calling the request function multiple times the buffer view was free'd repeatedly. I fixed this in pull #45.

@wjakob wjakob closed this as completed in 4bef0d3 Jan 17, 2016
wjakob pushed a commit that referenced this issue Jan 17, 2016
…med count->size to match NumPy naming (fixes #34)
wjakob pushed a commit that referenced this issue Jan 17, 2016
…med count->size to match NumPy naming (fixes #34)
EricCousineau-TRI added a commit to EricCousineau-TRI/pybind11 that referenced this issue Jan 15, 2020
…0-01-13

Merge 'upstream/master' (07e2259) from previous merge-base (12e8774)
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

No branches or pull requests

2 participants