Create sequences using initializer list by cowo78 · Pull Request #2451 · pybind/pybind11

cowo78 · 2020-09-01T14:42:56Z

To me it would be useful to be able to construct a few containers (i.e. tuple, list) using initializer lists (see #1669)

std::initializer_list (see pybind#1669)

YannickJadoul

Seems useful & harmless to me.

tests/test_pytypes.cpp

YannickJadoul · 2020-09-01T14:56:59Z

include/pybind11/pytypes.h

+    explicit tuple(std::initializer_list<object> init_list) : tuple(init_list.size()) {
+        size_t index {0};
+        for (const pybind11::object& item : init_list)
+            detail::tuple_accessor(*this, index++) = item;
+    }


With this, we could think about deprecating setting elements of a tuple, since tuples are supposed to be immutable once constructed?

Makes sense to me.

Nothing for this PR ofc :-)

tests/test_pytypes.py

initializer list.

test_pytypes also checks for new mixed types data returned.

YannickJadoul · 2020-09-02T09:58:40Z

Thanks for the updates, @cowo78!

Let's get a few more reviews in, and then this should be able to go in, as far as I'm concerned.

bstaletic · 2020-09-02T15:55:27Z

I would have preferred something more general. Why not allow std::array or boost::unordered_map? In C++20 we can do something like this:

explicit tuple(std::ranges::forward_range auto&& rng) : tuple(std::ranges::distance(rng)) {
    size_t i = 0;
    for(object&& o : rng) detail::tuple_accessor(*this, index++) = item;
}

Now I know C++20 is really really new, but any thoughts on doing something like that with enable_if and SFINAE blackmagic?

YannickJadoul · 2020-09-02T16:00:24Z

Is there a problem with accepting any <typename Container> that has container.begin() and container.end() of the right type? C++20 seems really recent :-/

bstaletic · 2020-09-02T16:04:19Z

If you accept any type with begin() and end(), you'll end up taking distance() of istream_view thus exhausting the view and then would try to iterate it. The minimal requirement needs to be "any container whose iterators are forward iterators".

YannickJadoul · 2020-09-02T16:45:56Z

Ok, right. What I tried to say: I like that idea so much that it would be a pity to only have it on C++20 ;)

Would you like to look into this, @cowo78, or should we merge this independently and have @bstaletic set up another PR? (Or @bstaletic could add to this PR, ofc)

bstaletic · 2020-09-02T17:51:56Z

Woo! My enable_if-foo isn't too bad!

#include <type_traits>
#include <iterator>
#include <utility>
template<typename... Ts> struct make_void { typedef void type;};
template<typename... Ts> using void_t = typename make_void<Ts...>::type;
template <typename T, typename = void>
struct is_iterable : std::false_type {};
template <typename T>
struct is_iterable<T, void_t<decltype(std::declval<T>().begin()),
                             decltype(std::declval<T>().end())>>
    : std::true_type {};

template <typename T,
	  typename std::enable_if<is_iterable<T>::value &&
				  std::is_base_of<std::forward_iterator_tag,
				                  typename std::iterator_traits<typename T::iterator>::iterator_category>::value,
				  int>::type = 0>
void f(T&&) {}

int main() {
	f(std::vector<int>{});
	f(std::list<int>{});
	f(std::set<int>{});
	f(std::initializer_list<int>{});
	f(std::map<int,int>{});
	//f(pybind11::dict{}); // pybind11 types don't have iterator typedef.
}

cowo78 · 2020-09-03T08:27:22Z

Ok, right. What I tried to say: I like that idea so much that it would be a pity to only have it on C++20 ;)

Would you like to look into this, @cowo78, or should we merge this independently and have @bstaletic set up another PR? (Or @bstaletic could add to this PR, ofc)

I won't argue about C++-20 since I did not (yet) look into the new features. I will try to provide something that allows using containers whose iterators are forward iterators.

Since we're dancing it would be also interesting to construct the containers using heterogeneous types i.e.
py::tuple(1, 1.3, "something")

bstaletic · 2020-09-03T09:21:38Z

I won't argue about C++-20 since I did not (yet) look into the new features. I will try to provide something that allows using containers whose iterators are forward iterators.

Thanks!

Since we're dancing it would be also interesting to construct the containers using heterogeneous types i.e.
py::tuple(1, 1.3, "something")

In C++17 that would be something like this:

template<typename ...Ts>
tuple(Ts&&... ts) : tuple(sizeof...(Ts)) {
    // C++17 fold expressions. C++11 would need some recursive template instantiations and would tank compile times.
    size_t index = 0;
    (..., (detail::tuple_accessor(*this, index++) = py::cast(std::forward<Ts>(ts))));
}

Questions:

Do we want py::tuple constructible from any iterable? Like folly::FBString?
Do we want py::tuple constructible from T...? If we use this one only, tuple(std::vector<int>{1,2,3}) compiles, but len(tuple) == 1 and tuple == ([1,2,3],).
Do we want both or only one of these?

Even though the variadic template version is more efficient than an equivalent initializer_list one, I think we should just go with the "any forward range" idea.

concept (only tuple right now). Provides 3 new constructors: initializer_list<py::object> to use heterogeneous py::objects initializer_list<T> to use homogeneous C++ objects container<T> to use C++ containers that provide a size() method and a forward iterator (see pybind#2451 discussion)

using also py::str. Added tests for WIP container based constructor for tuple.

cowo78 · 2020-09-03T11:19:12Z

Since we're dancing it would be also interesting to construct the containers using heterogeneous types i.e.
py::tuple(1, 1.3, "something")

In C++17 that would be something like this:
template<typename ...Ts>
tuple(Ts&&... ts) : tuple(sizeof...(Ts)) {
    // C++17 fold expressions. C++11 would need some recursive template instantiations and would tank compile times.
    size_t index = 0;
    (..., (detail::tuple_accessor(*this, index++) = py::cast(std::forward<Ts>(ts))));
}
Questions:

My 2c:

1. Do we want `py::tuple` constructible from any iterable? Like `folly::FBString`?

Yes.

2. Do we want `py::tuple` constructible from `T...`? If we use this one only, `tuple(std::vector<int>{1,2,3})` compiles, but `len(tuple) == 1` and `tuple == ([1,2,3],)`.

No. We want T, Ts... (at least 2 template args).

3. Do we want both or only one of these?

Both, so that you have the following options:

from initializer_listpy::object
from initializer_list<C++ type>
from C++ container
from heterogeneous C++ types passed as arguments

Even though the variadic template version is more efficient than an equivalent initializer_list one, I think we should just go with the "any forward range" idea.

Does not hurt to keep both, as it also gives an option to limit binary size.

YannickJadoul · 2020-09-03T13:46:05Z

Two things:

I propose to keep this PR small, so we can reasonably quickly merge the part we all agree on. So, let's just try to implement @bstaletic's more general proposal of having any forward-iterable class instead of initializer_list?
Constructing a tuple from variadic templated arguments already exists: py::make_tuple. So again, let's keep this simple and just implement the improvement to the original proposal, I'd say.

YannickJadoul · 2020-09-03T13:49:04Z

Clarification: by that I mean, we only add a constructor from SomeContainer<py::object>, as we shouldn't confuse the py::tuple interface for something that does a bunch of conversions; there's py::make_tuple for that, and if that needs to be adapted, it's another PR.
SomeContainer<py::object> should also match std::initializer_list<py::object>, I think, so no need to add that overload?

cowo78 · 2020-09-03T14:27:36Z

Two things:

* I propose to keep this PR small, so we can reasonably quickly merge the part we all agree on. So, let's just try to implement @bstaletic's more general proposal of having any forward-iterable class instead of `initializer_list`?

My 2c:

AFAIK move semantics do not work with initializer list (so will need a separate constructor for containers)
I want to use an initializer list with, i.e. py::int_ and py::float_ (need an initializer_listpy::object)
I want to use an initializer list with plain C++ object (need a templated initializer_list)

So, yes, stuff is bloating up here. It may very well be that my knowledge is not up to the task but I did not figure out how to do something more compact.

* Constructing a `tuple` from variadic templated arguments already exists: [`py::make_tuple`](https://github.com/pybind/pybind11/blob/4c36fb7b1236fce25e00b63f357ccc36dc006662/include/pybind11/cast.h#L1822-L1848). So again, let's keep this simple and just implement the improvement to the original proposal, I'd say.

Right, I forgot that.

YannickJadoul · 2020-09-03T14:34:06Z

AFAIK move semantics do not work with initializer list (so will need a separate constructor for containers)

Oh, I don't know about that. @bstaletic will, though, I expect @bstaletic?

I want to use an initializer list with plain C++ object (need a templated initializer_list)

I don't agree with adding that here, I think. It was also not part of your original proposal. py::tuple is the C++ interface to a Python tuple. That might be something for another PR, to maybe add to py::make_tuple. Or you just write your own function for that, in your personal project, ofc.

So, yes, stuff is bloating up here. It may very well be that my knowledge is not up to the task but I did not figure out how to do something more compact.

No, I'm not claiming that. I just want to keep things separated a bit.

cowo78 · 2020-09-03T14:51:48Z

I want to use an initializer list with plain C++ object (need a templated initializer_list)

I don't agree with adding that here, I think. It was also not part of your original proposal. py::tuple is the C++ interface to a Python tuple. That might be something for another PR, to maybe add to py::make_tuple. Or you just write your own function for that, in your personal project, ofc.

Well, it wasn't in the original proposal just because I used py::object for a start. I just think that both versions are useful. Your shout however.

So, yes, stuff is bloating up here. It may very well be that my knowledge is not up to the task but I did not figure out how to do something more compact.

No, I'm not claiming that. I just want to keep things separated a bit.

Again, I'm just a simple user so your shout here.

bstaletic · 2020-09-03T14:57:46Z

AFAIK move semantics do not work with initializer list (so will need a separate constructor for containers)

Oh, I don't know about that. @bstaletic will, though, I expect @bstaletic?

Sort of correct. An initializer_list<T> basically holds const T& and doesn't own the data. This means that std::move(il.back()) is the same as il.back(). This doesn't mean that things suddenly break.

YannickJadoul · 2020-09-03T15:03:57Z

Well, it wasn't in the original proposal just because I used py::object for a start. I just think that both versions are useful. Your shout however.

I'm also not the only one going over this, ofc. But here's my suggestion: adapt this PR to @bstaletic's suggestion (ForwardIterableContainer<py::object>, hopefully also working for initializer_list, let's see), and propose the other thing in a new PR if you want. I'm currently not a hug fan to merge it, but this will give us and the other maintainers some room to discuss what to do with it.

EDIT: What's @bstaletic's take on this?

bstaletic · 2020-09-03T15:13:41Z

This pull request is quickly losing focus and we're getting side-tracked. That aside, I'm not a fan of initializer_list<T>, because we have py::make_tuple. Like @YannickJadoul said, py::tuple::tuple(...) isn't meant to do random conversions. Anyway, this overload makes initializer_list<py::object> useless.

As for initializer_list<py::object>, it's functionality is covered by both, initializer_list<T> and my idea of "any forward range".

Also, my "any forward range" should probably have a constraint that is_base_of<handle, T::value_type>::value is true.

YannickJadoul · 2020-09-03T15:20:54Z

Actually, I'm quickly losing the value of constructing a py::tuple from an initializer_list, as well. There's py::make_tuple, which does exactly what you want, already (and it also works with py::object).

cowo78 · 2020-09-08T10:58:40Z

This pull request is quickly losing focus and we're getting side-tracked. That aside, I'm not a fan of initializer_list<T>, because we have py::make_tuple. Like @YannickJadoul said, py::tuple::tuple(...) isn't meant to do random conversions. Anyway, this overload makes initializer_list<py::object> useless.

As for initializer_list<py::object>, it's functionality is covered by both, initializer_list<T> and my idea of "any forward range".

Also, my "any forward range" should probably have a constraint that is_base_of<handle, T::value_type>::value is true.

Well, I think one can make use of py::tuple({1,2,3}). Currently this feature is not available (and py::make_tuple does not work this way with GCC 10.2.0) so the logic behind this PR.

initializer_list<py::object> and initializer_list<T> do not 100% overlap as with the templated version you cannot do `py::tuple({py::int_(1), py::float_(2.2)}).

The "forward range" concept of course is much better than the constrained "initializer_list" but it seems that you cannot use a templated rvalue constructor with an initializer list.

All of this said I think we can go back to a simple question: do we really need an initializer_list constructor since its functionality can basically be achieved with existing py::make_tuple? Since it's mostly sugar to provide a more intuitive (at least for me) usage pattern I'm just fine with either decision.

YannickJadoul · 2020-09-08T12:46:47Z

Well, I think one can make use of py::tuple({1,2,3}). Currently this feature is not available (and py::make_tuple does not work this way with GCC 10.2.0) so the logic behind this PR.

I'm, again, really not sure it's not py::tuple's job to do conversion. This is py::make_tuple's job, I think (though I'm not sure). At any rate, none of the other C++ classes for Python types do this, so we want to keep this as simple as possible. The original idea (with py::objects) is probably easy to merge in. But converting from a bunch of other things really needs another discussion, probably best in another PR.

initializer_list<py::object> and initializer_list<T> do not 100% overlap as with the templated version you cannot do `py::tuple({py::int_(1), py::float_(2.2)}).

The "forward range" concept of course is much better than the constrained "initializer_list" but it seems that you cannot use a templated rvalue constructor with an initializer list.

I'm going to pass this to @bstaletic, who tried out more. Can we not achieve this, @bstaletic?

All of this said I think we can go back to a simple question: do we really need an initializer_list constructor since its functionality can basically be achieved with existing py::make_tuple? Since it's mostly sugar to provide a more intuitive (at least for me) usage pattern I'm just fine with either decision.

I think it's nice to have a Python-interface to construct from a list of py::objects, though, so I wouldn't mind adding that constructor. The other one is then up for later debate, but I think these two issues can be separate?

based constructor only. Removed all other experimental constructors.

constructors work as expected.

cowo78 · 2020-10-08T09:42:03Z

I reverted all other constructor and left only a std::initializer_list<pybind11::object> based constructor for tuples, lists and sets.

joukewitteveen · 2021-03-04T14:17:35Z

include/pybind11/pytypes.h

        if (!m_ptr) pybind11_fail("Could not allocate tuple object!");
    }
+    /** \rst
+        Creates a ``tuple`` from an initializer list of object instances.


Maybe

Suggested change

Creates a ``tuple`` from an initializer list of object instances.

Creates a ``tuple`` from an initializer list of ``py::object`` instances.

just to be explicit? In addition, the added documentation on make_tuple in #2840 could be augmented with some remarks on the new ways to initialize a tuple. I didn't bother to document list and set there, because they are mutable (pybind11 offers ways to modify tuples, but that's besides the point).

Sure. Explicit is better than implicit.

joukewitteveen · 2021-03-04T14:18:56Z

tests/test_pytypes.cpp

+#include <array>
+#include <deque>
+#include <list>
+#include <set>
+#include <string>
+#include <vector>
+


Suggested change

#include <array>

#include <deque>

#include <list>

#include <set>

#include <string>

#include <vector>

I assume these are leftovers?

Yes, but leave the #include <string> as strings are used.

Giuseppe Corbelli added 2 commits September 1, 2020 16:36

pybind11::tuple and pybind11::list can be constructed using a

593047e

std::initializer_list (see pybind#1669)

Test initializer-list based constructors for tuple and list

586b9e5

YannickJadoul approved these changes Sep 1, 2020

View reviewed changes

YannickJadoul reviewed Sep 1, 2020

View reviewed changes

tests/test_pytypes.cpp Outdated Show resolved Hide resolved

YannickJadoul reviewed Sep 1, 2020

View reviewed changes

tests/test_pytypes.py Outdated Show resolved Hide resolved

Giuseppe Corbelli added 2 commits September 2, 2020 09:46

Test case creates a tuple and a list with mixed datatypes using

8e33d26

initializer list.

Refactored test_pytypes to account for pybind#2451 suggestions.

c9ac962

test_pytypes also checks for new mixed types data returned.

Giuseppe Corbelli added 4 commits September 3, 2020 12:52

Expanded the initializer_list constructor for tuple, list and set tests

4d8df0d

using also py::str. Added tests for WIP container based constructor for tuple.

Removed useless include.

bb3dfc1

Added double-line separator as per flake complaints.

d2b21d6

Added round braces in subobject initialization (clang warning).

c6665f2

YannickJadoul self-requested a review September 3, 2020 15:20

Giuseppe Corbelli added 2 commits October 8, 2020 11:38

tuple, list and set have a new std::initializer_list<pybind11::object>

5a943d8

based constructor only. Removed all other experimental constructors.

Test that tuple, list and set std::initializer_list<pybind11::object>

27eeb99

constructors work as expected.

henryiii added this to the v2.7 milestone Nov 5, 2020

joukewitteveen reviewed Mar 4, 2021

View reviewed changes

henryiii modified the milestones: v2.7, v2.8 Jul 16, 2021

joukewitteveen mentioned this pull request Aug 22, 2021

Add make_simple_namespace function and tests #2840

Merged

henryiii modified the milestones: v2.8, v2.9 Oct 4, 2021

rwgk mentioned this pull request Feb 10, 2023

FWD pybind11 google/pybind11clif#2451

Closed

	Creates a ``tuple`` from an initializer list of object instances.
	Creates a ``tuple`` from an initializer list of ``py::object`` instances.

Conversation

cowo78 commented Sep 1, 2020

Uh oh!

YannickJadoul left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

YannickJadoul Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

cowo78 Sep 2, 2020

Choose a reason for hiding this comment

Uh oh!

YannickJadoul Sep 2, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

YannickJadoul commented Sep 2, 2020

Uh oh!

bstaletic commented Sep 2, 2020

Uh oh!

YannickJadoul commented Sep 2, 2020

Uh oh!

bstaletic commented Sep 2, 2020

Uh oh!

YannickJadoul commented Sep 2, 2020

Uh oh!

bstaletic commented Sep 2, 2020

Uh oh!

cowo78 commented Sep 3, 2020

Uh oh!

bstaletic commented Sep 3, 2020

Uh oh!

cowo78 commented Sep 3, 2020

Uh oh!

YannickJadoul commented Sep 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YannickJadoul commented Sep 3, 2020

Uh oh!

cowo78 commented Sep 3, 2020

Uh oh!

YannickJadoul commented Sep 3, 2020

Uh oh!

cowo78 commented Sep 3, 2020

Uh oh!

bstaletic commented Sep 3, 2020

Uh oh!

YannickJadoul commented Sep 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bstaletic commented Sep 3, 2020

Uh oh!

YannickJadoul commented Sep 3, 2020

Uh oh!

cowo78 commented Sep 8, 2020

Uh oh!

YannickJadoul commented Sep 8, 2020

Uh oh!

cowo78 commented Oct 8, 2020

Uh oh!

joukewitteveen Mar 4, 2021

Choose a reason for hiding this comment

Uh oh!

cowo78 Mar 8, 2021

Choose a reason for hiding this comment

Uh oh!

joukewitteveen Mar 4, 2021

Choose a reason for hiding this comment

Uh oh!

cowo78 Mar 8, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

YannickJadoul commented Sep 3, 2020 •

edited

Loading

YannickJadoul commented Sep 3, 2020 •

edited

Loading