Adding bind_map. Adding key_error exception. #235

lyskov · 2016-06-11T04:11:39Z

Adding bind_map which provide minimal bindings for std::map. Adding key_error exception.

wjakob · 2016-06-15T19:09:50Z

Hi Sergey,

thank you for the PR. This is in my queue for the next release, but it may take me a bit to review it (I'm leaving for a vacation).

Cheers,
Wenzel

lyskov · 2016-06-15T20:07:34Z

Sure thing Wenzel! This is certainly not urgent. And please let me know if there is anything i can do to help with this.

Have a good vacation!

Sergey.

jagerman · 2016-07-15T20:45:26Z

include/pybind11/stl_bind.h

@@ -424,13 +424,18 @@ pybind11::class_<std::map<Key, T, Compare, Allocator>, holder_type> bind_map(pyb

    cl.def("__getitem__",
 		   [](Map &m, const KeyType &k) -> MappedType {
-			   if (m.count(k)) return m[k];
+			   if (m.count(k)) return m.at(k);


This could potentially require two lookups--one for the one for the count, and one for the element access, and the at() is a bit wasteful (as it checks existence again). The following would avoid both, while being const-usable:

auto it = m.find(k); if (it != m.end()) return *m; else throw pybind11::key_error();

Thank you for checking on this! Good point, I will path this!

The __delitem__ implementation can do the same thing as well, via m.erase(it);

jagerman · 2016-07-17T16:40:52Z

include/pybind11/stl_bind.h

+
+}
+
+template<typename Map, typename Class_, typename std::enable_if<std::is_const<typename Map::mapped_type>::value, int>::type = 0>


I think you could generalize this a bit by changing
std::is_const<typename Map::mapped_type>::value
to
!std::is_copy_assignable<Map::mapped_type>::value
which would still catch const values, but would also catch non-const types that don't have a copy assignment operator.

Good point!

wjakob · 2016-08-15T04:56:38Z

Hi Sergey,

could you rebase this onto master? Also, here are a few more comments:

The key iterator has landed, so __iter__ could make use of it.
There are various commented parts of the PR -- what about those?
It would be nice if there was at least some rudimentary documentation that mentioned the existence of stl_bind.h in the advanced part of the docs. Right now it's completely undocumented.

THanks,
Wenzel

lyskov · 2016-08-25T00:26:24Z

Re make_key_iterator: right, i just pushed the commit that uses it. Please let me know if this is the right way to do it.

@wjakob do i need to register Python testing functions somewhere to make sure that they called during the testing phase? Because right now it does not look like the testing functions is called at all (at least in my setup) (because assert in test_map_string_double should certainly fail due to different numbers of key's, - but it does not). Thought i do see parsing errors if i introduce them into test_stl_binders.py... thoughts?

lyskov · 2016-08-25T03:52:58Z

Looks like the tests run fine on appveyor... i guess it is a problem with my setup.

jagerman · 2016-08-26T00:51:33Z

Re make_key_iterator: right, i just pushed the commit that uses it. Please let me know if this is the right way to do it.

The __iter__ looks good to me (you could write it as: pybind11::make_key_iterator(m) if you prefer, but that's purely stylistic).

It would be nice to also add:

cl.def("items",
    [](Map &m) { return pybind11::make_iterator(m.begin(), m.end()); },
    pybind11::keep_alive<0, 1>() /* Essential: keep list alive while iterator exists */
);

to allow pairwise iteration from Python:

for k, v in mymap.items():
    print("%s = %s" % (k, v))

lyskov · 2016-08-26T21:40:34Z

Good idea about the 'items' @jagerman ! - I will add this shortly!

lyskov · 2016-08-26T23:00:01Z

@wjakob i think the last commit addressed the remaining issues, please let me know if there is anything else that needs to be done. Thanks!

wjakob · 2016-08-27T10:00:51Z

docs/advanced.rst

@@ -550,6 +550,18 @@ and the Python ``list``, ``set`` and ``dict`` data structures are automatically
 enabled. The types ``std::pair<>`` and ``std::tuple<>`` are already supported
 out of the box with just the core :file:`pybind11/pybind11.h` header.

+Alternatively it might be desirable to bind STL containers as native C++ classes
+therefor eliminating the need of converting back and forth between C++ representation


"thereforE" (or even better: just a comma)

lyskov · 2016-08-29T20:18:33Z

@wjakob thank you for looking this up! I updated the docs as you suggested and untabifyed the source files.

dean0x7d · 2016-08-29T20:21:43Z

docs/advanced.rst

@@ -979,6 +991,12 @@ exceptions:
 |                                      | accesses in ``__getitem__``, |
 |                                      | ``__setitem__``, etc.)       |
 +--------------------------------------+------------------------------+
+| :class:`pybind11::key_error`         | ``KeyError`` (used to        |


Duplicate: key_error is already documented a few lines down.

wjakob · 2016-08-29T20:34:57Z

This looks good to me now. Two more things: can you remove the duplicate docs pointed out by @dean0x7d and squash this into a single commit?
Thanks, Wenzel

jagerman · 2016-08-30T00:35:34Z

Just a heads up: it looks like stl_bind.h still has some tabs in it (on lines 445-448): https://travis-ci.org/pybind/pybind11/jobs/156090804#L287 (that was supposed to start triggering a build failure to let you see it right away, but needs #373 merged to start actually doing that).

Also a space-less if( on line 405.

lyskov · 2016-08-30T03:03:27Z

Ok, this is all should be fixed now.

@wjakob i did squash the commits into one as we'll. (Btw, i think GitHub now have option of doing this automatically when merging PR: after clicking 'merge' on confirmation button there is 'squash and merge' or something)

jagerman · 2016-08-30T16:22:45Z

This PR appears to only support std::map, but in a lot of cases std::unordered_map seems a better choice for C++11 code. It ought to be very easy to extend this PR to support unordered_map by doing something like the following (untested):

// In pybind11::detail namespace:
template <typename MapType, typename holder_type, typename... Args...> bind_map_type(module &m, const std::string &name, Args&&... args) {
// ... current bind_map() code
}


// In pybind11 namespace:

template <typename Key, typename T, typename Compare = std::less<Key>, typename Allocator = std::allocator<std::pair<const Key, T> >, typename holder_type = std::unique_ptr<std::map<Key, T, Compare, Allocator>>, typename... Args>
pybind11::class_<std::map<Key, T, Compare, Allocator>, holder_type> bind_map(pybind11::module &m, std::string const &name, Args&&... args) {
    return detail::bind_map_type<std::map<Key, T, Compare, Allocator>, holder_type>(m, name, std::forward<Args>(args)...);
}

template <typename Key, typename T, typename Hash = std::hash<Key>, KeyEqual = std::equal_to<Key>, typename Allocator = std::allocator<std::pair<const Key, T> >, typename holder_type = std::unique_ptr<std::unordered_map<Key, T, Hash, KeyEqual, Allocator>>, typename... Args>
pybind11::class_<std::unordered_map<Key, T, Hash, KeyEqual, Allocator>, holder_type> bind_unordered_map(pybind11::module &m, std::string const &name, Args&&... args) {
    return detail::bind_map_type<std::unordered_map<Key, T, Hash, KeyEqual, Allocator>, holder_type>(m, name, std::forward<Args>(args)...);
}

jagerman · 2016-08-30T16:34:44Z

~~That previous post isn't quite right--the std::unordered_map version needs to take Hash and KeyEqual parameters instead of the Compare parameter, but the idea is the same.~~ [Edit: fixed it].

Actually, a nicer alternative might be to just make bind_map take the map type, instead of the Key/T/Compare/Allocator template parameters, replacing the current bind_map interface with:

template <typename Map, typename holder_type = std::unique_ptr<Map>, typename... Args>
pybind11::class_<Map, holder_type> bind_map(module &m, const std::string &name, Args&&... args) {
    // ... same body as existing bind_map but without the "using Map = ..." line.
}

Then the caller can call it with any compatible map-like class, e.g.

pybind11::bind_map<std::unordered_map<...>>(m, "Map1");
pybind11::bind_map<std::map<...>>(m, "Map2");
pybind11::bind_map<MyCustomMapType>(m, "Map3");

lyskov · 2016-08-30T18:57:13Z

@jagerman good idea about generalizing this for different map types! I will keep this in mind for future PR's.

Re passing map type vs. passing type arguments: i think i explored that approach initially but in the end decided on passing on individual arguments instead (but not sure now what was the final reason for this). But i do agree that with map generalization passing one type might be a more general approach.

wjakob · 2016-09-05T07:33:55Z

Hi Sergey,

can you rebase this commit to the master branch? (can't be merged ATM).

Thanks,
Wenzel

wjakob · 2016-09-05T07:35:28Z

By the way: if the generalization to unordered maps is really as simple as @jagerman writes above, it would be a pity to not include it in this commit as well.

lyskov · 2016-09-05T21:14:48Z

@wjakob i just double checked: - the initial reason for passing arguments for map type is no longer relevant so @jagerman suggestion is now implemented and relevant tests for unordered_map is added. I also refactored bind_vector so it now take Vector type as template parameter instead of pair <T, Allocator> which should allow it to be used with vector-like types.

This PR is now rebased on current upstream so it is mergeable. Please let me know if there is anything else thats need to be done.

jagerman · 2016-09-06T02:39:23Z

The updated vector interface is nice, to keep it consistent with the map interface, but I think that's going to need an extra templated definition for backwards compatibility.

wjakob · 2016-09-06T03:13:19Z

This wasn't official API thus far (not documented in any way), so I think it's fine. Furthermore, we are heading towards a major release which can break compatibility with impunity.

(Also, I don't think it's possible to address this incompatibility with an extra template definition in a straightforward way)

lyskov · 2016-09-06T03:29:06Z

Great, thank you for taking care of this Wenzel!

wjakob force-pushed the master branch 4 times, most recently from 00cec38 to 5130212 Compare July 8, 2016 12:03

jagerman reviewed Jul 15, 2016
View reviewed changes

jagerman mentioned this pull request Jul 15, 2016

Add pybind11::make_key_iterator for map iteration #282

Merged

jagerman reviewed Jul 17, 2016
View reviewed changes

wjakob reviewed Aug 27, 2016
View reviewed changes

jagerman mentioned this pull request Aug 28, 2016

Check for style issues during docs build #369

Merged

dean0x7d reviewed Aug 29, 2016
View reviewed changes

rosetta-benchmark force-pushed the stl branch 3 times, most recently from 63236eb to c86faf6 Compare August 30, 2016 02:58

rosetta-benchmark force-pushed the stl branch 2 times, most recently from 89d550a to bb76756 Compare September 5, 2016 19:50

Adding bind_map

7520418

rosetta-benchmark force-pushed the stl branch from bb76756 to 7520418 Compare September 5, 2016 21:11

wjakob merged commit 07082ee into pybind:master Sep 6, 2016

lyskov deleted the stl branch September 6, 2016 03:29

rwgk mentioned this pull request Feb 9, 2023

FWD pybind11 google/pybind11clif#235

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding bind_map. Adding key_error exception. #235

Adding bind_map. Adding key_error exception. #235

lyskov commented Jun 11, 2016

wjakob commented Jun 15, 2016

lyskov commented Jun 15, 2016

jagerman Jul 15, 2016

lyskov Jul 15, 2016

jagerman Jul 15, 2016 •

edited

Loading

jagerman Jul 17, 2016

lyskov Jul 18, 2016

wjakob commented Aug 15, 2016

lyskov commented Aug 25, 2016

lyskov commented Aug 25, 2016

jagerman commented Aug 26, 2016

lyskov commented Aug 26, 2016

lyskov commented Aug 26, 2016

wjakob Aug 27, 2016

lyskov commented Aug 29, 2016

dean0x7d Aug 29, 2016

wjakob commented Aug 29, 2016

jagerman commented Aug 30, 2016 •

edited

Loading

lyskov commented Aug 30, 2016

jagerman commented Aug 30, 2016 •

edited

Loading

jagerman commented Aug 30, 2016 •

edited

Loading

lyskov commented Aug 30, 2016

wjakob commented Sep 5, 2016

wjakob commented Sep 5, 2016

lyskov commented Sep 5, 2016

jagerman commented Sep 6, 2016

wjakob commented Sep 6, 2016

lyskov commented Sep 6, 2016


		}

		template<typename Map, typename Class_, typename std::enable_if<std::is_const<typename Map::mapped_type>::value, int>::type = 0>

Adding bind_map. Adding key_error exception. #235

Adding bind_map. Adding key_error exception. #235

Conversation

lyskov commented Jun 11, 2016

wjakob commented Jun 15, 2016

lyskov commented Jun 15, 2016

jagerman Jul 15, 2016

Choose a reason for hiding this comment

lyskov Jul 15, 2016

Choose a reason for hiding this comment

jagerman Jul 15, 2016 • edited Loading

Choose a reason for hiding this comment

jagerman Jul 17, 2016

Choose a reason for hiding this comment

lyskov Jul 18, 2016

Choose a reason for hiding this comment

wjakob commented Aug 15, 2016

lyskov commented Aug 25, 2016

lyskov commented Aug 25, 2016

jagerman commented Aug 26, 2016

lyskov commented Aug 26, 2016

lyskov commented Aug 26, 2016

wjakob Aug 27, 2016

Choose a reason for hiding this comment

lyskov commented Aug 29, 2016

dean0x7d Aug 29, 2016

Choose a reason for hiding this comment

wjakob commented Aug 29, 2016

jagerman commented Aug 30, 2016 • edited Loading

lyskov commented Aug 30, 2016

jagerman commented Aug 30, 2016 • edited Loading

jagerman commented Aug 30, 2016 • edited Loading

lyskov commented Aug 30, 2016

wjakob commented Sep 5, 2016

wjakob commented Sep 5, 2016

lyskov commented Sep 5, 2016

jagerman commented Sep 6, 2016

wjakob commented Sep 6, 2016

lyskov commented Sep 6, 2016

jagerman Jul 15, 2016 •

edited

Loading

jagerman commented Aug 30, 2016 •

edited

Loading

jagerman commented Aug 30, 2016 •

edited

Loading

jagerman commented Aug 30, 2016 •

edited

Loading