-
Notifications
You must be signed in to change notification settings - Fork 55
Fix: Support to assign the complex number to SimpleArray #662
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
base: master
Are you sure you want to change the base?
Conversation
9d890ea to
7836ffb
Compare
ThreeMonth03
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yungyuc @tigercosmos Please review this pull request. Thanks.
| if constexpr (is_complex_v<T>) | ||
| { | ||
| if (py::isinstance(py_value, complex_class)) | ||
| { | ||
| arr_out.at(key) = py_value.cast<std_complex_t<T>>(); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When py_value is Complex or np.complex, casting to std::complex at first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The if/else code is unbalanced and hard to read:
if constexpr (is_complex_v<T>)
{
if (py::isinstance(py_value, complex_class))
{
arr_out.at(key) = py_value.cast<std_complex_t<T>>();
return;
}
}
arr_out.at(key) = py_value.cast<T>();
return;The following code is more concise and maintainable:
if (is_complex_v<T> && py::isinstance(py_value, complex_class))
{
arr_out.at(key) = py_value.cast<std_complex_t<T>>();
}
else
{
arr_out.at(key) = py_value.cast<T>();
}constexpr is unnecessary because py::isinstance(py_value, complex_class) needs to happen during runtime rather than compile time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, the program will return compile error if we don't use if constexpr.
template <typename T>
struct complex_element_type
{
};
template <typename T>
struct complex_element_type<Complex<T>>
{
using type = T;
};
template <typename T>
using complex_element_type_t = typename complex_element_type<T>::type;
template <typename T>
using std_complex_t = std::complex<complex_element_type_t<T>>;The following message is compile error.
/home/threemonth03/Downloads/modmesh/cpp/modmesh/buffer/pymod/wrap_SimpleArrayPlex.cpp:325:55: required from here
/home/threemonth03/Downloads/modmesh/cpp/modmesh/math/Complex.hpp:287:7: error: no type named ‘type’ in ‘struct modmesh::complex_element_type<double>’
In file included from /home/threemonth03/Downloads/modmesh/cpp/modmesh/math/math.hpp:8,
from /home/threemonth03/Downloads/modmesh/cpp/modmesh/buffer/SimpleArray.hpp:32,
from /home/threemonth03/Downloads/modmesh/cpp/modmesh/buffer/buffer.hpp:38,
from /home/threemonth03/Downloads/modmesh/cpp/modmesh/toggle/toggle.hpp:32,
from /home/threemonth03/Downloads/modmesh/cpp/modmesh/python/common.hpp:38,
from /home/threemonth03/Downloads/modmesh/cpp/modmesh/buffer/pymod/buffer_pymod.hpp:34,
from /home/threemonth03/Downloads/modmesh/cpp/modmesh/buffer/pymod/wrap_SimpleArray.cpp:29:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. Please figure out why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. Please figure out why.
The error message is simple, it caused from the missing alias in generic template.
template <typename T>
struct complex_element_type
{
};However, if we add the alias using type = T; in generic template, it would deduce to a wrong type, so I think I should redesign the template , such that it might look like using convert_to_std_t = something...;.
cpp/modmesh/math/Complex.hpp
Outdated
| template <typename T> | ||
| struct complex_element_type | ||
| { | ||
| }; | ||
|
|
||
| template <typename T> | ||
| struct complex_element_type<Complex<T>> | ||
| { | ||
| using type = T; | ||
| }; | ||
|
|
||
| template <typename T> | ||
| using complex_element_type_t = typename complex_element_type<T>::type; | ||
|
|
||
| template <typename T> | ||
| using std_complex_t = std::complex<complex_element_type_t<T>>; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the type of complex<T>, return std::complex<T>
| .def("__complex__", | ||
| [](const wrapped_type & self) | ||
| { | ||
| return self.to_std_complex(); | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add an interface to support casting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job. Points to address:
- Balance the if/else to improve readability for the value assignment.
- Write an informative summary in the commit log.
| if constexpr (is_complex_v<T>) | ||
| { | ||
| if (py::isinstance(py_value, complex_class)) | ||
| { | ||
| arr_out.at(key) = py_value.cast<std_complex_t<T>>(); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The if/else code is unbalanced and hard to read:
if constexpr (is_complex_v<T>)
{
if (py::isinstance(py_value, complex_class))
{
arr_out.at(key) = py_value.cast<std_complex_t<T>>();
return;
}
}
arr_out.at(key) = py_value.cast<T>();
return;The following code is more concise and maintainable:
if (is_complex_v<T> && py::isinstance(py_value, complex_class))
{
arr_out.at(key) = py_value.cast<std_complex_t<T>>();
}
else
{
arr_out.at(key) = py_value.cast<T>();
}constexpr is unnecessary because py::isinstance(py_value, complex_class) needs to happen during runtime rather than compile time.
7836ffb to
9792bac
Compare
ThreeMonth03
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Balance the if/else to improve readability for the value assignment.
- Write an informative summary in the commit log.
@yungyuc Please review this pull request again. Thanks.
| template <typename T> | ||
| struct convert_to_std | ||
| { | ||
| using type = T; | ||
| }; | ||
|
|
||
| template <typename T> | ||
| struct convert_to_std<Complex<T>> | ||
| { | ||
| using type = std::complex<T>; | ||
| }; | ||
|
|
||
| template <typename T> | ||
| using convert_to_std_t = typename convert_to_std<T>::type; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The template could identify not only modmesh::complex<T> but also standard type.
|
|
||
| if (is_complex_v<T> && py::isinstance(py_value, complex_class)) | ||
| { | ||
| arr_out.at(key) = py_value.cast<convert_to_std_t<T>>(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove if constexpr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please balance if/else as I mentioned in the previous review:
if (/* ... */)
{
// ...
}
else
{
// ...
}The early return should be factored outside the branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice that if we replace the branch with the following code, it also could work.
arr_out.at(key) = py_value.cast<convert_to_std_t<T>>();However, if the input is modmesh::complex<T>, it casts to std::complex<T> at first, then it casts to modmesh::complex<T>. I'm not sure which design is better.
yungyuc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commit log looks good, but you did not balance the branch.
|
|
||
| if (is_complex_v<T> && py::isinstance(py_value, complex_class)) | ||
| { | ||
| arr_out.at(key) = py_value.cast<convert_to_std_t<T>>(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please balance if/else as I mentioned in the previous review:
if (/* ... */)
{
// ...
}
else
{
// ...
}The early return should be factored outside the branch.
a9ae9be to
a3f3b10
Compare
…the argument of `np.complex` - Check whether `py::object py_value` is `np.complex`. - If `py::object py_value` is `np.complex`, cast `py::object py_value` to `modmesh::complex<T>`.
a3f3b10 to
f93b669
Compare
ThreeMonth03
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yungyuc Please review this pr. Thanks.
|
|
||
| if (is_complex_v<T> && py::isinstance(py_value, complex_class)) | ||
| { | ||
| arr_out.at(key) = py_value.cast<convert_to_std_t<T>>(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice that if we replace the branch with the following code, it also could work.
arr_out.at(key) = py_value.cast<convert_to_std_t<T>>();However, if the input is modmesh::complex<T>, it casts to std::complex<T> at first, then it casts to modmesh::complex<T>. I'm not sure which design is better.
Problem
In issue #604 , when we assign values from
np.complex128orComplextoSimpleArrayComplex128, the cast of python instance fails.Solution
The function
__setitem__inwrap_SimpleArray.cppcalls &property_helper::setitem_parser, and the fuunctioncalls &property_helper::setitem_parserchecks if the value is number based on the following logic:This condition ignores whether
py_valueisnp.complex128orComplex, it just check whetherpy_valueismodmesh.complex128.