8000 BUG: np.dtype(ctypes.Structure) does not respect _pack_ field · Issue #10532 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

BUG: np.dtype(ctypes.Structure) does not respect _pack_ field #10532

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.

Sign up for GitHub

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

Jump to bottom
Closed
eric-wieser opened this issue Feb 6, 2018 · 10 comments
Closed

BUG: np.dtype(ctypes.Structure) does not respect _pack_ field #10532

eric-wieser opened this issue Feb 6, 2018 · 10 comments
Labels
00 - Bug component: numpy._core

Comments

@eric-wieser
Copy link
Member
eric-wieser commented Feb 6, 2018 •
edited
Loading 
class Foo(ctypes.Structure):
    _fields_ = [('one', ctypes.c_uint8), ('two', ctypes.c_uint32)]
    _pack_ = 2
>>> ctypes.sizeof(Foo())  # packed to alignment 2
6
>>> np.dtype(Foo).itemsize  # default alignment incorrectly used
8

Fix should be fairly straightforward by changing:

numpy/numpy/core/_dtype_ctypes.py

Lines 35 to 46 in 44810f0

def _from_ctypes_structure(t):
# TODO: gh-10533, gh-10532
fields = []
for item in t._fields_:
if len(item) > 2:
raise TypeError(
"ctypes bitfields have no dtype equivalent")
fname, ftyp = item
fields.append((fname, dtype_from_ctypes_type(ftyp)))
# by default, ctypes structs are aligned
return np.dtype(fields, align=True)

@eric-wieser eric-wieser mentioned this issue Feb 7, 2018
Closed
@eric-wieser eric-wieser mentioned this issue Aug 11, 2018
Closed
@tylerjereddy
Copy link
Contributor 8000

Incidentally, I've been thinking a lot about using ctypes to define custom dtypes lately, since Python-based custom dtype definition is currently on our roadmap. I wonder how many other issues there are with trying to reconstitute the NumPy version of PyTypeObject struct from a Python object subclassed from a ctypes.Structure.

@eric-wieser
Copy link
Member Author

I don't fully understand what you're suggesting - but right now using ctypes as an exchange format is a pretty poor path for anything, because the objects are hard to inspect:

@tylerjereddy
Copy link
Contributor

I basically mean doing what you just did above, but extending it to comprehensively define a useful custom dtype that inherits from ctypes.Structure. Looks like you're way ahead of me and already have CPython PRs open!

@eric-wieser
Copy link
Member Author

To be clear - by custom dtype, do you mean structured dtype, or are you envisaging applying it to things like fixed-pointer numbers / auto-differentiating scalars / etc?

@tylerjereddy
Copy link
Contributor

More the auto-differentiating scalars and other custom dtypes of that nature, like those supporting units or missing integer support.

So:

class AmazingDtype(ctypes.Structure):
    # this is an amazing third-party NumPy dtype
    # that implements automatic forward differentiation
    # without touching a single C file by using the
    # Python standard library ctypes.Structure to fill
    # in all the requisite structure members of the type class
    # needed to do useful things as a NumPy dtype
    pass

@mattip
Copy link
Member
mattip commented Aug 13, 2018

I would not like to encourage deeper use of ctypes. The code behind them has many open issues. The package defines yet-another-dsl for describing c-like interfaces. It has so many layers it will always be slow and buggy. Why not directly inherit from a new dtypes.baseclass that returns NotImplemented where needed instead?

@eric-wieser eric-wieser mentioned this issue Oct 26, 2018
Merged
@eric-wieser
Copy link
Member Author

I'm with @mattip here. We don't want to built on top of ctypes - only provide compatibility with it.

@eric-wieser
Copy link
Member Author

As of #12254, this should be fairly straightforward to fix.

@eric-wieser eric-wieser mentioned this issue Nov 2, 2018
Closed
@bmakos bmakos mentioned this issue Nov 6, 2018
Merged
@bmakos
Copy link
Contributor
bmakos commented Nov 6, 2018 •
edited
Loading

I submitted a pull request with a fix for this bug. I am new to git and I am struggling a bit to understand how I am supposed to do this exactly.

This is the request:
https://github.com/numpy/numpy/pull/12342

I assume a code review would be next?

To fix the issue I saw in the dtypes documentation how to use a dictionary to provide the offset values. In my fix in case the class has a pack attribute I will provide the offset values from it, otherwise use the previously existing code.

@eric-wieser
Copy link
Member Author

@tylerjereddy:

Incidentally, I've been thinking a lot about using ctypes to define custom dtypes lately,

A neat but rather pointless trick

@np.dtype
class Foo(ctypes.Structure):
    _fields_ = [
        ('x', ctypes.c_uint32),
        ('y', ctypes.c_uint32)
    ]

liwt31 pushed a commit to liwt31/numpy that referenced this issue Nov 19, 2018
@bmakos @liwt31
BUG: Fix for np.dtype(ctypes.Structure) does not respect _pack_ field
5bd6584 
See numpy#10532
liwt31 pushed a commit to liwt31/numpy that referenced this issue Nov 19, 2018
@bmakos @liwt31
BUG: Fix for np.dtype(ctypes.Structure) does not respect _pack_ field
f3a0ce0 
See numpy#10532
Made several changes to offset and size calulation. First pull request
was far from correct.
liwt31 pushed a commit to liwt31/numpy that referenced this issue Nov 19, 2018
@bmakos @liwt31
BUG: Fix for np.dtype(ctypes.Structure) does not respect _pack_ field
e1232e4 
See numpy#10532
Cosmetic changes + added an additional test with a more complex
structure.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
00 - Bug component: numpy._core
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@eric-wieser @mattip @bmakos @tylerjereddy
0