Allow std::complex field with PYBIND11_NUMPY_DTYPE #831
Conversation
This exposed a few underlying issues: 1. is_pod_struct was too strict to allow this. I've relaxed it to require only trivially copyable and standard layout, rather than POD (which additionally requires a trivial constructor, which std::complex violates). Should the name be changed too? There might also need to be some documentation to warn the user not to do something stupid with a class that has invariants. 2. format_descriptor<std::complex<T>>::format() returned numpy format strings instead of PEP3118 format strings, but register_dtype feeds format codes of its fields to _dtype_from_pep3118. I've changed it to return PEP3118 format codes. format_descriptor is a public type, so this may be considered an incompatible change. 3. register_structured_dtype tried to be smart about whether to mark fields as unaligned (with ^). However, it's examining the C++ alignment, rather than what numpy (or possibly PEP3118) thinks the alignment should be. For complex values those are different. I've made it mark all fields as ^ unconditionally, which should always be safe even if they are aligned, because we explicitly mark the padding.
|
The test failures seem to be because GCC 4.8 doesn't implement std::is_trivially_copyable. What's the best way to fix this? |
|
Would |
It's not really the same thing. A class can be trivially copyable (and even POD) without being trivially destructible (if it has a deleted destructor). Maybe the check for trivially copyable should just be omitted for GCC 4? Would it likely cause any instances of type deduction actually going wrong, or would it merely weaken the static_assert? I'm not sure where the right place to handle compiler-specific hacks is though. |
|
Sure, but it covers at least part of the trivially copyable requirements. Actually, as a gcc 4.8 workaround, you might get pretty close by checking both of: |
include/pybind11/numpy.h
Outdated
| @@ -244,7 +244,8 @@ template <typename T> struct is_complex : std::false_type { }; | |||
| template <typename T> struct is_complex<std::complex<T>> : std::true_type { }; | |||
|
|
|||
| template <typename T> using is_pod_struct = all_of< | |||
| std::is_pod<T>, // since we're accessing directly in memory we need a POD type | |||
| std::is_standard_layout<T>, // since we're accessing directly in memory we need a standard layout type | |||
| std::is_trivially_copyable<T>, | |||
There was a problem hiding this comment.
For libstcd++ < 5 (which doesn't have is_trivially_copyable) this workaround might work:
std::is_standard_layout<T>,
#if !defined(__GNUG__) || defined(__clang__) || __GNUC__ >= 5
std::is_trivially_copyable<T>,
#else
std::is_trivially_destructible<T>, std::has_trivial_copy_constructor<T>,
#endif
satisfies_none_of<...It's not quite the same as trivially copyable, but it ought to be close enough in practice.
There was a problem hiding this comment.
Closer still:
std::is_trivially_destructible<T>, satisifies_any_of<T, std::has_trivial_copy_constructor, std::has_trivial_copy_assign>
libstdc++ doesn't appear to have an equivalent has_trivial_move_constructor, but the above should handle most cases.
It's not sufficient, of course, but having a non-deleted, trivial destructor is necessary. |
GCC 4 doesn't provide std::is_trivially_copyable.
Interesting. Neither Clang 3.8 nor GCC 5.4 seems to enforce that (they allow a deleted destructor). I've implemented your suggestion, except requiring both has_trivial_copy_constructor and has_trivial_copy_assign rather than either, since the trivially constructible concept requires both (except that it allows them to be deleted - maybe that's what you were getting at by only requiring one?) |
|
That is what I was trying to get at. If at least one of them is trivial, even if the other isn't, it means there is at least one trivial path, which seems good enough for the workaround in this case. Otherwise the gcc 4.x check ends up being more restrictive than the newer compiler check. |
|
Fair enough, I'll change it to what you suggested. |
Require either a trivial copy constructor or a trivial copy assignment, but not both. This is slightly more relaxed than being TriviallyCopyable, which requires that both are either trivial or deleted, and that at least one is not deleted. With this check, one of them could be user-provided.
This moves the ^ in the format string (to specify unaligned) to
outside the `T{}` where it is sure to be parsed correctly. This is not
strictly necessary yet, but it paves the way for pybind#832.
|
One of the reasons for requiring POD (hence trivial constructible types) is that accessing numpy arrays involves reinterpret casts which may break user-defined invariants set up in the constructors. I think it's quite reasonable to require it in general? Another alternative to swapping std::is_pod for std::is_standard_layout, I guess, would be to introduce another trait (detail::is_pod?) that would cover POD types plus std::complex as a special case. |
That would be ideal, but I don't see a feasible means of doing it, e.g. if I have a struct which has a struct member which itself has a std::complex member. |
|
It's not even a question of nested structs. The POD assertion is applied to the first argument of I will look at adding something to the documentation to warn the user against doing stupid things. |
|
By the way, any experience seeing similar errors to the VS2015 failures? I'm not really sure where to start debugging that, not having a Windows machine. |
Not saying we should do it, but it's actually kind of feasible, because you can iterate over flattened struct fields at compile time, I happened to watch a cppcon16 talk about it just yesterday :) It's a bit of an overkill, but an amusing hack nonetheless. |
For some reason, returning the 'value' constexpr char array as a string would yield an empty string for format_descriptor<float>. I tried a few variants of the constructor and this is the first variation I found that worked.
That looks like some seriously deep magic, but it looks like it depends on aggregate initialization - which I think breaks down once std::complex is part of the struct (but I haven't been following C++14 or 17, so I might be wrong). Either way, it does seem overkill :-) |
|
I've just watched the CppCon talk. I now think it may actually be feasible (if still rather overkill), and as a bonus it is probably even possible to generate numpy format strings (at compile time!) for user-defined structures without the user registering them first (with the caveat that the field names can't be captured). |
|
Yea, I think it's possible but you wouldn't get field names which is quite sad. Also, you won't be able to ignore certain fields, and the entire structure will be always flattened... All in all, this LGTM, looks like we'll have to drop |
include/pybind11/common.h
Outdated
| static constexpr const char value[2] = { c, '\0' }; | ||
| static std::string format() { return std::string(1, c); } | ||
| static constexpr const char c1 = "?bBhHiIqQfdgZZZ"[detail::is_fmt_numeric<T>::index]; | ||
| static constexpr const char c2 = "\0\0\0\0\0\0\0\0\0\0\0\0fdg"[detail::is_fmt_numeric<T>::index]; |
There was a problem hiding this comment.
I have to say, I'm not a big fan of this part. I think it would be cleaner to just remove FDG from here and specialize the whole format_descriptor struct for std::complex inside complex.h.
There was a problem hiding this comment.
(Especially in light of the extra hack required below for MSVC).
There was a problem hiding this comment.
Done. Now for some reason the VS2017 x86 Python 3.6 build (but none of the others) is failing, and there is no indication of what the error is. Also, Travis doesn't seem to have been run at all. Any ideas?
There was a problem hiding this comment.
The AppVeyor issue is most likely random: #792. Not sure what happened to Travis.
There was a problem hiding this comment.
Yep, looks like it was just random failures. Appveyor is still running the VS2015 builds, but it should all be working now.
At the request of @jagerman, a separate specialization of format_descriptor is implemented for std::complex. This may run into MSVC 2015 bugs again - will put back the workaround if it fails.
This is to go with the weakened is_pod_struct static_assert.
|
Merged, thanks! |
This exposed a few underlying issues:
is_pod_struct was too strict to allow this. I've relaxed it to
require only trivially copyable and standard layout, rather than POD
(which additionally requires a trivial constructor, which std::complex
violates). Should the name be changed too? There might also need to be
some documentation to warn the user not to do something stupid with a
class that has invariants.
format_descriptor<std::complex>::format() returned numpy format
strings instead of PEP3118 format strings, but register_dtype
feeds format codes of its fields to _dtype_from_pep3118. I've changed it
to return PEP3118 format codes. format_descriptor is a public type, so
this may be considered an incompatible change.
register_structured_dtype tried to be smart about whether to mark
fields as unaligned (with ^). However, it's examining the C++ alignment,
rather than what numpy (or possibly PEP3118) thinks the alignment should
be. For complex values those are different. I've made it mark all fields
as ^ unconditionally, which should always be safe even if they are
aligned, because we explicitly mark the padding.
This addresses #799.