Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Commit 8635df8

Browse files
sarlinpesarlinpe
authored
Fix bug in UpdateFromDict (#231)
* Improve UpdateFromDict * Cleanup other functions
1 parent 422a474 commit 8635df8

1 file changed

Lines changed: 58 additions & 57 deletions

File tree

‎pycolmap/helpers.h‎

Lines changed: 58 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88

99
#include "pycolmap/log_exceptions.h"
1010

11+
#include <exception>
1112
#include <memory>
1213
#include <regex>
1314
#include <sstream>
@@ -33,17 +34,16 @@ const Eigen::IOFormat vec_fmt(Eigen::StreamPrecision,
3334

3435
template <typename T>
3536
inline T pyStringToEnum(const py::enum_<T>& enm, const std::string& value) {
36-
auto values = enm.attr("__members__").template cast<py::dict>();
37-
auto strVal = py::str(value);
38-
if (values.contains(strVal)) {
39-
return T(values[strVal].template cast<T>());
37+
const auto values = enm.attr("__members__").template cast<py::dict>();
38+
const auto str_val = py::str(value);
39+
if (values.contains(str_val)) {
40+
return T(values[str_val].template cast<T>());
4041
}
41-
std::string msg =
42+
const std::string msg =
4243
"Invalid string value " + value + " for enum " +
4344
std::string(enm.attr("__name__").template cast<std::string>());
4445
THROW_EXCEPTION(std::out_of_range, msg.c_str());
45-
T t;
46-
return t;
46+
return T();
4747
}
4848

4949
template <typename T>
@@ -54,25 +54,31 @@ inline void AddStringToEnumConstructor(py::enum_<T>& enm) {
5454
py::implicitly_convertible<std::string, T>();
5555
}
5656

57-
inline void UpdateFromDict(py::object& self, py::dict& dict) {
58-
for (auto& it : dict) {
57+
inline void UpdateFromDict(py::object& self, const py::dict& dict) {
58+
for (const auto& it : dict) {
59+
if (!py::isinstance<py::str>(it.first)) {
60+
const std::string msg = "Dictionary key is not a string: " +
61+
py::str(it.first).template cast<std::string>();
62+
THROW_EXCEPTION(std::invalid_argument, msg.c_str());
63+
}
64+
const py::str name = py::reinterpret_borrow<py::str>(it.first);
65+
const py::handle& value = it.second;
66+
const auto attr = self.attr(name);
5967
try {
60-
if (py::hasattr(self.attr(it.first), "mergedict")) {
61-
self.attr(it.first).attr("mergedict").attr("__call__")(it.second);
68+
if (py::hasattr(attr, "mergedict") && py::isinstance<py::dict>(value)) {
69+
attr.attr("mergedict").attr("__call__")(value);
6270
} else {
63-
self.attr(it.first) = it.second;
71+
self.attr(name) = value;
6472
}
6573
} catch (const py::error_already_set& ex) {
6674
if (ex.matches(PyExc_TypeError)) {
6775
// If fail we try bases of the class
68-
py::list bases = self.attr(it.first)
69-
.attr("__class__")
70-
.attr("__bases__")
71-
.cast<py::list>();
76+
const py::list bases =
77+
attr.attr("__class__").attr("__bases__").cast<py::list>();
7278
bool success_on_base = false;
7379
for (auto& base : bases) {
7480
try {
75-
self.attr(it.first) = base(it.second);
81+
self.attr(name) = base(value);
7682
success_on_base = true;
7783
break;
7884
} catch (const py::error_already_set&) {
@@ -86,31 +92,27 @@ inline void UpdateFromDict(py::object& self, py::dict& dict) {
8692
ss << self.attr("__class__")
8793
.attr("__name__")
8894
.template cast<std::string>()
89-
<< "." << py::str(it.first).template cast<std::string>()
90-
<< ": Could not convert "
91-
<< py::type::of(it.second.cast<py::object>())
92-
.attr("__name__")
93-
.template cast<std::string>()
94-
<< ": " << py::str(it.second).template cast<std::string>() << " to '"
95-
<< py::type::of(self.attr(it.first))
95+
<< "." << name.template cast<std::string>() << ": Could not convert "
96+
<< py::type::of(value.cast<py::object>())
9697
.attr("__name__")
9798
.template cast<std::string>()
99+
<< ": " << py::str(value).template cast<std::string>() << " to '"
100+
<< py::type::of(attr).attr("__name__").template cast<std::string>()
98101
<< "'.";
99102
// We write the err message to give info even if exceptions
100103
// is catched outside, e.g. in function overload resolve
101104
LOG(ERROR) << "Internal TypeError: " << ss.str();
102105
throw(py::type_error(std::string("Failed to merge dict into class: ") +
103106
"Could not assign " +
104-
py::str(it.first).template cast<std::string>()));
107+
name.template cast<std::string>()));
105108
} else if (ex.matches(PyExc_AttributeError) &&
106109
py::str(ex.value()).cast<std::string>() ==
107110
std::string("can't set attribute")) {
108111
std::stringstream ss;
109112
ss << self.attr("__class__")
110113
.attr("__name__")
111114
.template cast<std::string>()
112-
<< "." << py::str(it.first).template cast<std::string>()
113-
<< " defined readonly.";
115+
<< "." << name.template cast<std::string>() << " defined readonly.";
114116
throw py::attribute_error(ss.str());
115117
} else if (ex.matches(PyExc_AttributeError)) {
116118
LOG(ERROR) << "Internal AttributeError: "
@@ -125,33 +127,32 @@ inline void UpdateFromDict(py::object& self, py::dict& dict) {
125127
}
126128
}
127129

130+
bool AttributeIsFunction(const std::string& name, const py::object& attribute) {
131+
return (name.find("__") == 0 || name.rfind("__") != std::string::npos ||
132+
py::hasattr(attribute, "__func__") ||
133+
py::hasattr(attribute, "__call__"));
134+
}
135+
128136
template <typename T, typename... options>
129137
inline py::dict ConvertToDict(const T& self) {
130-
auto pyself = py::cast(self);
138+
const auto pyself = py::cast(self);
131139
py::dict dict;
132-
for (auto& handle : pyself.attr("__dir__")()) {
133-
std::string attribute = py::str(handle);
134-
auto member = pyself.attr(attribute.c_str());
135-
if (attribute.find("__") != 0 &&
136-
attribute.rfind("__") == std::string::npos &&
137-
!py::hasattr(member, "__func__")) {
138-
if (py::hasattr(member, "todict")) {
139-
dict[attribute.c_str()] =
140-
member.attr("todict").attr("__call__")().template cast<py::dict>();
141-
} else {
142-
dict[attribute.c_str()] = member;
143-
}
140+
for (const auto& handle : pyself.attr("__dir__")()) {
141+
const py::str name = py::reinterpret_borrow<py::str>(handle);
142+
const auto attribute = pyself.attr(name);
143+
if (AttributeIsFunction(name, attribute)) {
144+
continue;
145+
}
146+
if (py::hasattr(attribute, "todict")) {
147+
dict[name] =
148+
attribute.attr("todict").attr("__call__")().template cast<py::dict>();
149+
} else {
150+
dict[name] = attribute;
144151
}
145152
}
146153
return dict;
147154
}
148155

149-
bool AttributeIsFunction(const std::string& name, const py::object& attribute) {
150-
return (name.find("__") == 0 || name.rfind("__") != std::string::npos ||
151-
py::hasattr(attribute, "__func__") ||
152-
py::hasattr(attribute, "__call__"));
153-
}
154-
155156
template <typename T, typename... options>
156157
inline std::string CreateSummary(const T& self, bool write_type) {
157158
std::stringstream ss;
@@ -161,38 +162,38 @@ inline std::string CreateSummary(const T& self, bool write_type) {
161162
ss << pyself.attr("__class__").attr("__name__").template cast<std::string>()
162163
<< ":";
163164
for (auto& handle : pyself.attr("__dir__")()) {
164-
std::string attribute = py::str(handle);
165-
py::object member;
165+
const py::str name = py::reinterpret_borrow<py::str>(handle);
166+
py::object attribute;
166167
try {
167-
member = pyself.attr(attribute.c_str());
168+
attribute = pyself.attr(name);
168169
} catch (const std::exception& e) {
169170
// Some properties are not valid for some uninitialized objects.
170171
continue;
171172
}
172-
if (AttributeIsFunction(attribute, member)) {
173+
if (AttributeIsFunction(name, attribute)) {
173174
continue;
174175
}
175176
ss << "\n";
176177
if (!after_subsummary) {
177178
ss << prefix;
178179
}
179-
ss << attribute;
180-
if (py::hasattr(member, "summary")) {
181-
std::string summ = member.attr("summary")
180+
ss << attribute.template cast<std::string>();
181+
if (py::hasattr(attribute, "summary")) {
182+
std::string summ = attribute.attr("summary")
182183
.attr("__call__")(write_type)
183184
.template cast<std::string>();
184185
summ = std::regex_replace(summ, std::regex("\n"), "\n" + prefix);
185186
ss << ": " << summ;
186187
} else {
187188
if (write_type) {
188189
const std::string type_str =
189-
py::str(py::type::of(member).attr("__name__"));
190+
py::str(py::type::of(attribute).attr("__name__"));
190191
ss << ": " << type_str;
191192
after_subsummary = true;
192193
}
193-
std::string value = py::str(member);
194-
if (value.length() > 80 && py::hasattr(member, "__len__")) {
195-
const int length = member.attr("__len__")().template cast<int>();
194+
std::string value = py::str(attribute);
195+
if (value.length() > 80 && py::hasattr(attribute, "__len__")) {
196+
const int length = attribute.attr("__len__")().template cast<int>();
196197
value = StringPrintf(
197198
"%c ... %d elements ... %c", value.front(), length, value.back());
198199
}

0 commit comments

Comments
 (0)