numpy: Provide concrete size aliases, test equivalence for `dtype(...).num` #1329

EricCousineau-TRI · 2018-03-20T21:04:22Z

See the linked issue for context.

Note that this is split into two commits:

Introduces a simple explicit test on dtypes to guarantee that they match for concrete dtypes, and then some assurances on platform-dependent sizes. (No changes to numpy.h)
Mirrors the concrete type aliases (e.g. NPY_INT32). This solves an issue where py::dtype::of<int64_t>().attr("num") did not match to NumPy's definition of NPY_INT64.

TBH, I can understand some hesitation for incorporating the second commit. However, I would like to see the first commit make it in - took a bit to understand this issue and debug.

Todo:

Rebase once review is done.

EricCousineau-TRI · 2018-03-20T21:12:27Z

Patch between running python -m pytest -s test_numpy_array.py on first commit vs. second commit:

diff --git a/tmp/a.txt b/tmp/b.txt
index cda8c6e..23fd223 100644
--- a/tmp/a.txt
+++ b/tmp/b.txt
@@ -18,9 +18,7 @@ test_numpy_array.py <DtypeSizeCheck name='short' size_cpp=2 size_numpy=2 dtype=i
 <DtypeCheck numpy=int32 pybind11=int32>
 <DtypeCheck numpy=uint32 pybind11=uint32>
 <DtypeCheck numpy=int64 pybind11=int64>
-NOTE: typenum mismatch for <DtypeCheck numpy=int64 pybind11=int64>: 7 != 9
 <DtypeCheck numpy=uint64 pybind11=uint64>
-NOTE: typenum mismatch for <DtypeCheck numpy=uint64 pybind11=uint64>: 8 != 10
 .................................
 
-========================== 33 passed in 0.08 seconds ===========================
+========================== 33 passed in 0.07 seconds ===========================

jagerman · 2018-03-21T20:17:15Z

include/pybind11/numpy.h

+    return (sizeof(Concrete) == sizeof(Check1)) ? check1 :
+               (sizeof(Concrete) == sizeof(Check2)) ? check2 :
+                 (sizeof(Concrete) == sizeof(Check3)) ? check3 : -1;
+}


Here's a template that makes this a little more generic, along with a compile-time check for the error case (rather than returning -1). It also requires passing the arguments as an { ... } array because that seemed simpler.

using namespace pybind11::detail; template <typename T> struct same_size { template <typename U> using as = bool_constant<sizeof(T) == sizeof(U)>; }; template <typename Concrete, typename... Check> constexpr int platform_lookup(std::array<int, sizeof...(Check)> codes) { using code_index = std::integral_constant<int, constexpr_first<same_size<Concrete>::template as, Check...>()>; static_assert(code_index::value != sizeof...(Check), "Unable to match type on this platform"); return codes[code_index::value]; } int main() { // fails (short not matched): //int bar = platform_lookup<short, int, unsigned, unsigned long, int>({4, 9, 11, 13}); int foo = platform_lookup<std::uint32_t, unsigned int, unsigned long>({8,9}); }

Unfortunately, C++11 doesn't like multiple statements in the function - it deems it non-constexpr :(

Will briefly tinker with it to see if I can add some indirection to preserve the constexpr-ness.

Hup, was wrong: Just needed to make the array<> argument const array<>.
Thanks!

jagerman · 2018-03-21T20:22:45Z

include/pybind11/numpy.h

+            NPY_USHORT_, NPY_UINT_, NPY_ULONG_),
+        NPY_INT64_ = platform_lookup<int64_t, int, long, long long>(
+            NPY_INT_, NPY_LONG_, NPY_LONGLONG_),
+        NPY_UINT64_ = platform_lookup<uint64_t, uint, unsigned long, unsigned long long>(


uint isn't a standard C or C++ type (even if commonly available); that should be unsigned int, or if you're feeling lazy, just unsigned (though some people strongly dislike the latter).

The int32_t etc. types are more portable, but should still technically be prefixed with std:: (we should also have a #include <cstdint> in here to properly bring them in).

It looks like uint isn't available on MSVC.

Done. Thanks!

jagerman · 2018-03-21T20:31:14Z

It looks like the i386 tests broke, as well (one of the tests is coming up with a 4 instead of an 8).

EricCousineau-TRI · 2018-03-22T01:00:55Z

Reverted a portion of my change to see if this is something I introduced vs. something to be fixed.

The discrepancies here between the numpy size (4) and its dtype (int64) seem very disturbing though... like I made a mistake

<DtypeSizeCheck name='long long' size_cpp=8 size_numpy=4 dtype=int64>

EDIT: Realized that I'm using dtype.alignment, when I should be using dtype.itemsize. Trying that out now.

EricCousineau-TRI · 2018-03-22T07:21:56Z

TravisCI passed, but now AppVeyor seems to have failed due to some flakes?

Failure mode A:

Command executed with exception: 
==> WARNING: A newer version of conda exists. <==
  current version: 4.4.10
  latest version: 4.5.0
Please update conda by running
    $ conda update -n base conda
if exist "tests\test_cmake_build" type tests\test_cmake_build\*.log*
The system cannot find the file specified.
Command exited with code 1

Failure mode B:

c:\projects\pybind11\include\pybind11\numpy.h(152): fatal error C1001: An internal error has occurred in the compiler. [C:\projects\pybind11\tests\pybind11_tests.vcxproj]
  (compiler file 'msc1.cpp', line 1468)
   To work around this problem, try simplifying or changing the program near the locations listed above.
  Please choose the Technical Support command on the Visual C++ 
   Help menu, or open the Technical Support help file for more information (compiling source file C:\projects\pybind11\tests\test_eigen.cpp)
  Internal Compiler Error in C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\bin\x86_amd64\CL.exe.  You will be prompted to send an error report to Microsoft later.
  test_enum.cpp
cl : Command line error D8040: error creating or communicating with child process [C:\projects\pybind11\tests\pybind11_tests.vcxproj]
Command exited with code 1
if exist "tests\test_cmake_build" type tests\test_cmake_build\*.log*
The system cannot find the file specified.

Should I just close + re-open the PR to retrigger the builds?

jagerman · 2018-03-22T15:11:43Z

I found the easiest thing was to set up a free appveyor account and attach it to your own repository; then you can start, restart, etc. builds as required.

I'm not exactly sure what we need to do to fix Failure A, but that certainly isn't caused by your PR.

Failure B is more serious: it looks like MSVC 2015 hates my template generalization (though 2017 seems fine with it). You could try this version:

template <int, typename...> struct matches_sizeof_impl : std::integral_constant<int, -1> {};
template <int N, typename T, typename A, typename... B>
struct matches_sizeof_impl<N, T, A, B...> : std::integral_constant<int,
    sizeof(T) == sizeof(A) ? N : matches_sizeof_impl<N+1, T, B...>::value> {};
template <typename T, typename... A> using matches_sizeof_index = matches_sizeof_impl<0, T, A...>;

template <typename Concrete, typename... Check, typename code_index = matches_sizeof_index<Concrete, Check...>>
constexpr int platform_lookup(const std::array<int, sizeof...(Check)> &codes) {
    static_assert(code_index::value >= 0, "Unable to match type on this platform");
    return codes[code_index::value];
}

It's getting a bit messier, but at least it keeps that compilation failure if the type isn't found.

jagerman · 2018-03-22T15:25:05Z

Here's a better implementation; this makes first_true_index generic enough now that it could (and should!) go into pybind11/detail/common.h:

template <int N, bool... Bool> struct first_true_impl : std::integral_constant<int, -1> {};
template <int N, bool B, bool... BMore> struct first_true_impl<N, B, BMore...> :
    std::integral_constant<int, B ? N : first_true_impl<N+1, BMore...>::value> {};
/// Takes a parameter pack of bool values, return the index of the first one that is true or -1 if
/// none of the template arguments are true.
template <bool... B> using first_true_index = first_true_impl<0, B...>;

template <typename Concrete, typename... Check, typename code_index = first_true_index<sizeof(Concrete) == sizeof(Check)...>>
constexpr int platform_lookup(const std::array<int, sizeof...(Check)> &codes) {
    static_assert(code_index::value >= 0, "Unable to match type on this platform");
    return codes[code_index::value];
}

EricCousineau-TRI · 2018-04-23T01:53:27Z

Done - sorry about the delay!

EricCousineau-TRI · 2018-05-11T04:18:14Z

Just rebased - sorry for the drawn out delay!

Actually ended up needing this, as implicit conversions for py::type_caster for arrays requires that the registered dtype conversion (PyArray_RegisterCastFunc) be precisely np.int64; dtype::of<int> yields np.int32, and dtype::of<std::int64_t> does not yield the same typenum as np.int64.

olegsinavski · 2018-11-30T01:39:12Z

@EricCousineau-TRI, great work! I've run into this issue recently. I got into that by passing pybind int64_t array into OpenCV functions. Without this fix you'll get "data type = 9 is not supported" Exception. Any plans on merging this soon?

EricCousineau-TRI · 2018-11-30T14:55:19Z

Lemme see if I can fix these pesky MSVC issues...

EDIT: Rebased (just to see if it was fixed by other things);
if that fails, will see if it's OK if it's not having to index into a constexpr std::array<>, but rather a tuple:
EricCousineau-TRI@2d2c142

EDIT 2: Looks like the original bits failed, but only with VS 2015. Trying the different approach.

… do not exactly match dtype(...).num

Test for dtype checks now succeed without warnings

EricCousineau-TRI · 2018-12-01T20:54:33Z

K, finally found the magic incantation to make MSVC happy.

@jagerman Might you have a chance to give this another glance?

eacousineau · 2019-06-19T12:33:35Z

Hup! It looks like AppVeyor is happy now, huzzah! (~~I don't think I did anything special~~, aside from have Wenzel's update?)
EDIT: Nevermind... didn't read my separate account's latest comment :P

@jagerman or @wjakob Can I ask if this might be good to merge?
(It may or may not also relate a more recent issue, #1806)

wjakob · 2019-07-23T11:17:13Z

This looks good to me -- merged! :)

EricCousineau-TRI · 2019-07-23T14:26:39Z

Awesome, thanks!

YannickJadoul

Came across this, related to #1908 (& #1806)

YannickJadoul · 2020-07-17T18:06:12Z

include/pybind11/numpy.h

+        npy_api::NPY_BYTE_,   npy_api::NPY_UBYTE_,   npy_api::NPY_INT16_,    npy_api::NPY_UINT16_,
+        npy_api::NPY_INT32_,  npy_api::NPY_UINT32_,  npy_api::NPY_INT64_,    npy_api::NPY_UINT64_,


Is this actually correct? Normally, it should be the other way around, int16, int32, and int64 being aliases, and l should mean long, q longlong, ...?

See e.g.:

https://numpy.org/doc/stable/reference/arrays.scalars.html#built-in-scalar-types

https://github.com/numpy/numpy/blob/6ef5ec39cdfaf77aa4600ec2e3bf9f679a4fd527/numpy/core/include/numpy/npy_common.h#L545

https://github.com/numpy/numpy/blob/6ef5ec39cdfaf77aa4600ec2e3bf9f679a4fd527/numpy/core/include/numpy/ndarraytypes.h#L121

https://github.com/numpy/numpy/blob/6ef5ec39cdfaf77aa4600ec2e3bf9f679a4fd527/numpy/core/code_generators/generate_umath.py#L204

Good point! I've (explicitly) cross-ref'd your mention in the issue:
#1908 (comment)

The dumb question is, what's the correct testing fix? (it's been a while since I wrote this...)

I don't know. Still trying to figure out. I actually came across this by accident and wanted to make sure I'm correct, before starting to dig further into this! :-)

To make things more complex, I just found this as well:

>>> np.dtype('i1') dtype('int8') >>> np.dtype('i2') dtype('int16') >>> np.dtype('i4') dtype('int32') >>> np.dtype('i8') dtype('int64') >>> np.dtype('u8') dtype('uint64')

But yes, maybe let's discuss further in #1908 (though I'm not yet convinced that that one is actually an issue); easier to have things grouped in one place.

EricCousineau-TRI mentioned this pull request Mar 20, 2018

numpy_dtype_user: Pave way for downstream libs to define user dtypes RobotLocomotion/pybind11#12

Closed

4 tasks

jagerman reviewed Mar 21, 2018

View reviewed changes

EricCousineau-TRI added a commit to EricCousineau-TRI/pybind11 that referenced this pull request Apr 8, 2018

Squash pick of pybind#1329

d3c3cd4

EricCousineau-TRI force-pushed the issue/1328 branch from 9630d04 to cd496ce Compare May 11, 2018 04:15

olegsinavski mentioned this pull request Nov 30, 2018

numpy: Provide concrete size aliases, test equivalence for `dtype(...).num braincorp/pybind11#1

Merged

numpy: Add test for explicit dtype checks. At present, int64 + uint64…

ee660a0

… do not exactly match dtype(...).num

EricCousineau-TRI force-pushed the issue/1328 branch 2 times, most recently from 28fadb3 to 64e896a Compare November 30, 2018 23:25

numpy: Provide concrete size aliases

8f28b8e

Test for dtype checks now succeed without warnings

EricCousineau-TRI force-pushed the issue/1328 branch from 64e896a to 8f28b8e Compare December 1, 2018 12:31

wjakob force-pushed the master branch from c4321d3 to cf36e3d Compare June 11, 2019 21:29

eacousineau mentioned this pull request Jun 19, 2019

Wrong buffer format returned for similar numpy arrays passed to pybind11 function #1806

Closed

wjakob merged commit 4a3464f into pybind:master Jul 23, 2019

YannickJadoul reviewed Jul 17, 2020

View reviewed changes

EricCousineau-TRI mentioned this pull request Jul 17, 2020

Did you know that py::buffer_info::format has a different meaning on Windows? #1908

Open

rwgk mentioned this pull request Feb 9, 2023

FWD pybind11 google/pybind11clif#1329

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

numpy: Provide concrete size aliases, test equivalence for `dtype(...).num` #1329

numpy: Provide concrete size aliases, test equivalence for `dtype(...).num` #1329

EricCousineau-TRI commented Mar 20, 2018 •

edited

Loading

EricCousineau-TRI commented Mar 20, 2018 •

edited

Loading

jagerman Mar 21, 2018

EricCousineau-TRI Mar 22, 2018

EricCousineau-TRI Mar 22, 2018

jagerman Mar 21, 2018

jagerman Mar 21, 2018

EricCousineau-TRI Mar 22, 2018

jagerman commented Mar 21, 2018

EricCousineau-TRI commented Mar 22, 2018 •

edited

Loading

EricCousineau-TRI commented Mar 22, 2018

jagerman commented Mar 22, 2018 •

edited

Loading

jagerman commented Mar 22, 2018 •

edited

Loading

EricCousineau-TRI commented Apr 23, 2018

EricCousineau-TRI commented May 11, 2018 •

edited

Loading

olegsinavski commented Nov 30, 2018

EricCousineau-TRI commented Nov 30, 2018 •

edited

Loading

EricCousineau-TRI commented Dec 1, 2018

eacousineau commented Jun 19, 2019 •

edited

Loading

wjakob commented Jul 23, 2019

EricCousineau-TRI commented Jul 23, 2019

YannickJadoul left a comment

YannickJadoul Jul 17, 2020

EricCousineau-TRI Jul 17, 2020

YannickJadoul Jul 18, 2020

		npy_api::NPY_BYTE_, npy_api::NPY_UBYTE_, npy_api::NPY_INT16_, npy_api::NPY_UINT16_,
		npy_api::NPY_INT32_, npy_api::NPY_UINT32_, npy_api::NPY_INT64_, npy_api::NPY_UINT64_,

numpy: Provide concrete size aliases, test equivalence for dtype(...).num #1329

numpy: Provide concrete size aliases, test equivalence for dtype(...).num #1329

Conversation

EricCousineau-TRI commented Mar 20, 2018 • edited Loading

EricCousineau-TRI commented Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jagerman commented Mar 21, 2018

EricCousineau-TRI commented Mar 22, 2018 • edited Loading

EricCousineau-TRI commented Mar 22, 2018

jagerman commented Mar 22, 2018 • edited Loading

jagerman commented Mar 22, 2018 • edited Loading

EricCousineau-TRI commented Apr 23, 2018

EricCousineau-TRI commented May 11, 2018 • edited Loading

olegsinavski commented Nov 30, 2018

EricCousineau-TRI commented Nov 30, 2018 • edited Loading

EricCousineau-TRI commented Dec 1, 2018

eacousineau commented Jun 19, 2019 • edited Loading

wjakob commented Jul 23, 2019

EricCousineau-TRI commented Jul 23, 2019

YannickJadoul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

numpy: Provide concrete size aliases, test equivalence for `dtype(...).num` #1329

numpy: Provide concrete size aliases, test equivalence for `dtype(...).num` #1329

EricCousineau-TRI commented Mar 20, 2018 •

edited

Loading

EricCousineau-TRI commented Mar 20, 2018 •

edited

Loading

EricCousineau-TRI commented Mar 22, 2018 •

edited

Loading

jagerman commented Mar 22, 2018 •

edited

Loading

jagerman commented Mar 22, 2018 •

edited

Loading

EricCousineau-TRI commented May 11, 2018 •

edited

Loading

EricCousineau-TRI commented Nov 30, 2018 •

edited

Loading

eacousineau commented Jun 19, 2019 •

edited

Loading