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.

Sign up for GitHub

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

Add .keys and .values to bind_map #3310

Merged
merged 4 commits into from
Oct 1, 2021
Merged

Add .keys and .values to bind_map #3310

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7c16873
Add `.keys` and `.values` to bind_map
bmerry Sep 22, 2021
7582a27
bind_map: support any object in __contains__
bmerry Sep 30, 2021
1823edf
Take py::object by const reference in __contains__
bmerry Sep 30, 2021
9422f17
Removing stray `py::` (detected via interactive testing in Google env…
Sep 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 75 additions & 3 deletions include/pybind11/stl_bind.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -595,13 +595,33 @@ template <typename Map, typename Class_> auto map_if_insertion_operator(Class_ &
);
}

template<typename Map>
henryiii marked this conversation as resolved.
Show resolved Hide resolved
struct keys_view
{
Map &map;
};

template<typename Map>
struct values_view
{
Map &map;
};

template<typename Map>
struct items_view
{
Map &map;
};

PYBIND11_NAMESPACE_END(detail)

template <typename Map, typename holder_type = std::unique_ptr<Map>, typename... Args>
class_<Map, holder_type> bind_map(handle scope, const std::string &name, Args&&... args) {
using KeyType = typename Map::key_type;
using MappedType = typename Map::mapped_type;
using KeysView = detail::keys_view<Map>;
using ValuesView = detail::values_view<Map>;
using ItemsView = detail::items_view<Map>;
using Class_ = class_<Map, holder_type>;

// If either type is a non-module-local bound type then make the map binding non-local as well;
Expand All @@ -615,6 +635,12 @@ class_<Map, holder_type> bind_map(handle scope, const std::string &name, Args&&.
}

Class_ cl(scope, name.c_str(), pybind11::module_local(local), std::forward<Args>(args)...);
class_<KeysView> keys_view(
scope, ("KeysView[" + name + "]").c_str(), pybind11::module_local(local));
class_<ValuesView> values_view(
scope, ("ValuesView[" + name + "]").c_str(), pybind11::module_local(local));
class_<ItemsView> items_view(
scope, ("ItemsView[" + name + "]").c_str(), pybind11::module_local(local));
Comment on lines +638 to +643
Copy link

Choose a reason for hiding this comment

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

This naming seems to produce "erroneous" names for stub generation and subsequent parsing by Pylance or mypy, etc... Also sphinx autodoc has issues parsing it:

reading sources... [ 40%] references/python
WARNING: invalid signature for autoclass ('depthai::BoardConfig.ItemsView[GPIOMap]')

Thoughts on renaming to *View_{name}, with an underscore? Seems to work well afterwards.

Example
Before:

class GPIOMap:
    def __init__(self) -> None: ...
    def items(self) -> BoardConfig.ItemsView[GPIOMap]: ...
    def keys(self) -> BoardConfig.KeysView[GPIOMap]: ...
    def values(self) -> BoardConfig.ValuesView[GPIOMap]: ...
    def __bool__(self) -> bool: ...
    @overload
    def __contains__(self, arg0: int) -> bool: ...
    @overload
    def __contains__(self, arg0: object) -> bool: ...
    def __delitem__(self, arg0: int) -> None: ...
    def __getitem__(self, arg0: int) -> BoardConfig.GPIO: ...
    def __iter__(self) -> Iterator: ...
    def __len__(self) -> int: ...
    def __setitem__(self, arg0: int, arg1: BoardConfig.GPIO) -> None: ...

class ItemsView[GPIOMap]:
    def __init__(self, *args, **kwargs) -> None: ...
    def __iter__(self) -> Iterator: ...
    def __len__(self) -> int: ...
...

After:

class GPIOMap:
    def __init__(self) -> None: ...
    def items(self) -> BoardConfig.ItemsView_GPIOMap: ...
    def keys(self) -> BoardConfig.KeysView_GPIOMap: ...
    def values(self) -> BoardConfig.ValuesView_GPIOMap: ...
    def __bool__(self) -> bool: ...
    @overload
    def __contains__(self, arg0: int) -> bool: ...
    @overload
    def __contains__(self, arg0: object) -> bool: ...
    def __delitem__(self, arg0: int) -> None: ...
    def __getitem__(self, arg0: int) -> BoardConfig.GPIO: ...
    def __iter__(self) -> Iterator: ...
    def __len__(self) -> int: ...
    def __setitem__(self, arg0: int, arg1: BoardConfig.GPIO) -> None: ...

class ItemsView_GPIOMap:
    def __init__(self, *args, **kwargs) -> None: ...
    def __iter__(self) -> Iterator: ...
    def __len__(self) -> int: ...
...

Copy link
Collaborator

Choose a reason for hiding this comment

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

@themarpe sounds good. Please add a PR.

Copy link
Collaborator

@Skylion007 Skylion007 Jun 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@themarpe Actually, hold on. Why is it having any issues? It should be generating a typing.KeysView . Is there some other reason here why it is struggling to parse it correctly?

Copy link

@themarpe themarpe Jun 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@Skylion007 Thanks - will do so soon.
EDIT: Opened the PR

I think its mainly from the *View[name] naming. I think the square brackets gets at it. Works okay for mypy & Sphinx when renamed to something different


cl.def(init<>());

Expand All @@ -628,12 +654,22 @@ class_<Map, holder_type> bind_map(handle scope, const std::string &name, Args&&.

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

cl.def("keys",
[](Map &m) { return KeysView{m}; },
keep_alive<0, 1>() /* Essential: keep map alive while view exists */
);

cl.def("values",
[](Map &m) { return ValuesView{m}; },
keep_alive<0, 1>() /* Essential: keep map alive while view exists */
);

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

cl.def("__getitem__",
Expand All @@ -654,6 +690,8 @@ class_<Map, holder_type> bind_map(handle scope, const std::string &name, Args&&.
return true;
}
);
// Fallback for when the object is not of the key type
cl.def("__contains__", [](Map &, const object &) -> bool { return false; });

// Assignment provided only if the type is copyable
detail::map_assignment<Map, Class_>(cl);
Expand All @@ -669,6 +707,40 @@ class_<Map, holder_type> bind_map(handle scope, const std::string &name, Args&&.

cl.def("__len__", &Map::size);

keys_view.def("__len__", [](KeysView &view) { return view.map.size(); });
keys_view.def("__iter__",
[](KeysView &view) {
return make_key_iterator(view.map.begin(), view.map.end());
},
keep_alive<0, 1>() /* Essential: keep view alive while iterator exists */
);
keys_view.def("__contains__",
[](KeysView &view, const KeyType &k) -> bool {
auto it = view.map.find(k);
if (it == view.map.end())
return false;
return true;
}
);
// Fallback for when the object is not of the key type
keys_view.def("__contains__", [](KeysView &, const object &) -> bool { return false; });

values_view.def("__len__", [](ValuesView &view) { return view.map.size(); });
values_view.def("__iter__",
[](ValuesView &view) {
return make_value_iterator(view.map.begin(), view.map.end());
},
keep_alive<0, 1>() /* Essential: keep view alive while iterator exists */
);

items_view.def("__len__", [](ItemsView &view) { return view.map.size(); });
items_view.def("__iter__",
[](ItemsView &view) {
return make_iterator(view.map.begin(), view.map.end());
},
keep_alive<0, 1>() /* Essential: keep view alive while iterator exists */
);

return cl;
}

Expand Down
30 changes: 29 additions & 1 deletion tests/test_stl_binders.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,15 +160,43 @@ def test_map_string_double():
mm["b"] = 2.5

assert list(mm) == ["a", "b"]
assert list(mm.items()) == [("a", 1), ("b", 2.5)]
assert str(mm) == "MapStringDouble{a: 1, b: 2.5}"
assert "b" in mm
assert "c" not in mm
assert 123 not in mm

# Check that keys, values, items are views, not merely iterable
keys = mm.keys()
values = mm.values()
items = mm.items()
assert list(keys) == ["a", "b"]
assert len(keys) == 2
assert "a" in keys
assert "c" not in keys
assert 123 not in keys
assert list(items) == [("a", 1), ("b", 2.5)]
assert len(items) == 2
assert ("b", 2.5) in items
assert "hello" not in items
assert ("b", 2.5, None) not in items
assert list(values) == [1, 2.5]
assert len(values) == 2
assert 1 in values
assert 2 not in values
# Check that views update when the map is updated
mm["c"] = -1
assert list(keys) == ["a", "b", "c"]
assert list(values) == [1, 2.5, -1]
assert list(items) == [("a", 1), ("b", 2.5), ("c", -1)]

um = m.UnorderedMapStringDouble()
um["ua"] = 1.1
um["ub"] = 2.6

assert sorted(list(um)) == ["ua", "ub"]
assert list(um.keys()) == list(um)
assert sorted(list(um.items())) == [("ua", 1.1), ("ub", 2.6)]
assert list(zip(um.keys(), um.values())) == list(um.items())
assert "UnorderedMapStringDouble" in str(um)


Expand Down