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

Create sequences using initializer list #2451

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

cowo78
Copy link

@cowo78 cowo78 commented Sep 1, 2020

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

Copy link
Collaborator

@YannickJadoul YannickJadoul left a comment

Choose a reason for hiding this comment

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

Seems useful & harmless to me.

Comment on lines 1220 to 1224
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;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense to me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nothing for this PR ofc :-)

tests/test_pytypes.py Outdated Show resolved Hide resolved
Giuseppe Corbelli added 2 commits September 2, 2020 09:46
test_pytypes also checks for new mixed types data returned.
@YannickJadoul
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Author

cowo78 commented Sep 3, 2020

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
Copy link
Collaborator

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:

  1. Do we want py::tuple constructible from any iterable? Like folly::FBString?
  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],).
  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.

Giuseppe Corbelli added 4 commits September 3, 2020 12:52
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
Copy link
Author

cowo78 commented Sep 3, 2020

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
Copy link
Collaborator

YannickJadoul commented Sep 3, 2020

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
Copy link
Collaborator

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
Copy link
Author

cowo78 commented Sep 3, 2020

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
Copy link
Collaborator

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
Copy link
Author

cowo78 commented Sep 3, 2020

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
Copy link
Collaborator

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
Copy link
Collaborator

YannickJadoul commented Sep 3, 2020

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
Copy link
Collaborator

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 YannickJadoul self-requested a review September 3, 2020 15:20
@YannickJadoul
Copy link
Collaborator

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
Copy link
Author

cowo78 commented Sep 8, 2020

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
Copy link
Collaborator

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?

Giuseppe Corbelli added 2 commits October 8, 2020 11:38
based constructor only. Removed all other experimental constructors.
@cowo78
Copy link
Author

cowo78 commented Oct 8, 2020

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

@henryiii henryiii added this to the v2.7 milestone Nov 5, 2020
explicit tuple(size_t size = 0) : object(PyTuple_New((ssize_t) size), stolen_t{}) {
if (!m_ptr) pybind11_fail("Could not allocate tuple object!");
}
/** \rst
Creates a ``tuple`` from an initializer list of object instances.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Sure. Explicit is better than implicit.

Comment on lines +12 to +18
#include <array>
#include <deque>
#include <list>
#include <set>
#include <string>
#include <vector>

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#include <array>
#include <deque>
#include <list>
#include <set>
#include <string>
#include <vector>

I assume these are leftovers?

Copy link
Author

Choose a reason for hiding this comment

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

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

@henryiii henryiii modified the milestones: v2.7, v2.8 Jul 16, 2021
@henryiii henryiii modified the milestones: v2.8, v2.9 Oct 4, 2021
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.

5 participants