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

Allow std::complex field with PYBIND11_NUMPY_DTYPE #831

Merged
merged 9 commits into from
May 10, 2017

Conversation

bmerry
Copy link
Contributor

@bmerry bmerry commented May 4, 2017

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>::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.

This addresses #799.

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.
@bmerry
Copy link
Contributor Author

bmerry commented May 4, 2017

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?

@jagerman
Copy link
Member

jagerman commented May 4, 2017

Would std::is_trivially_destructible work?

@bmerry
Copy link
Contributor Author

bmerry commented May 4, 2017

Would std::is_trivially_destructible work?

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.

@jagerman
Copy link
Member

jagerman commented May 4, 2017

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:
std::has_trivial_copy_constructor and std::is_trivially_destructible.

@@ -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>,
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

@jagerman
Copy link
Member

jagerman commented May 4, 2017

A class can be trivially copyable (and even POD) without being trivially destructible (if it has a deleted destructor).

It's not sufficient, of course, but having a non-deleted, trivial destructor is necessary.

GCC 4 doesn't provide std::is_trivially_copyable.
@bmerry
Copy link
Contributor Author

bmerry commented May 4, 2017

It's not sufficient, of course, but having a non-deleted, trivial destructor is necessary.

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?)

@jagerman
Copy link
Member

jagerman commented May 4, 2017

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.

@bmerry
Copy link
Contributor Author

bmerry commented May 4, 2017

Fair enough, I'll change it to what you suggested.

bmerry added 2 commits May 4, 2017 18:22
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.
@aldanor
Copy link
Member

aldanor commented May 5, 2017

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.

@jagerman
Copy link
Member

jagerman commented May 5, 2017

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.

@bmerry
Copy link
Contributor Author

bmerry commented May 5, 2017

It's not even a question of nested structs. The POD assertion is applied to the first argument of PYBIND11_NUMPY_DTYPE, not the individual fields, so just a struct containing std::complex fails because the complex member causes the whole struct not to be POD. We could add assertions that the individual members are POD-or-complex, but then we run back into the issue of nested structures. And there may be other types where the user provides a default constructor but where it still makes sense to share with numpy.

I will look at adding something to the documentation to warn the user against doing stupid things.

@bmerry
Copy link
Contributor Author

bmerry commented May 5, 2017

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.

@aldanor
Copy link
Member

aldanor commented May 5, 2017

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.

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.
@bmerry
Copy link
Contributor Author

bmerry commented May 6, 2017

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 :)

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 :-)

@bmerry
Copy link
Contributor Author

bmerry commented May 6, 2017

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).

@aldanor
Copy link
Member

aldanor commented May 7, 2017

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 is_pod requirement for this to work anyway.

@aldanor
Copy link
Member

aldanor commented May 8, 2017

@bmerry numpy/numpy#9054

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];
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

(Especially in light of the extra hack required below for MSVC).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

The AppVeyor issue is most likely random: #792. Not sure what happened to Travis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, looks like it was just random failures. Appveyor is still running the VS2015 builds, but it should all be working now.

bmerry added 3 commits May 8, 2017 09:53
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.
@dean0x7d
Copy link
Member

Looks like merging #832 caused some conflicts here. @bmerry could you resolve those? Other than, this looks good to go.

@bmerry
Copy link
Contributor Author

bmerry commented May 10, 2017

Looks like merging #832 caused some conflicts here. @bmerry could you resolve those? Other than, this looks good to go.

Yes, I was expecting that. I've just pushed a merge commit.

@dean0x7d dean0x7d merged commit b82c0f0 into pybind:master May 10, 2017
@dean0x7d
Copy link
Member

Merged, thanks!

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.

4 participants