Complex py wrap #452

j8xixo12 · 2025-01-02T16:07:12Z

In this I have added modmesh complex implementation python wrapper, and containing python unit test code.
And it is in preparation for further fft GUI related development.

j8xixo12 · 2025-01-02T16:08:24Z

cpp/modmesh/math.hpp

-template <typename T, typename = std::enable_if_t<std::is_arithmetic<T>::value>>
-struct ComplexImpl
-{
-    T real_v;
-    T imag_v;
-
-    explicit ComplexImpl()
-        : ComplexImpl(0.0, 0.0)
-    {
-    }
-
-    explicit ComplexImpl(T r, T i)
-        : real_v(r)
-        , imag_v(i)
-    {
-    }
-
-    explicit ComplexImpl(const ComplexImpl & other)
-        : real_v(other.real_v)
-        , imag_v(other.imag_v)
-    {
-    }
-
-    ComplexImpl operator+(const ComplexImpl & other) const
-    {
-        return ComplexImpl(real_v + other.real_v, imag_v + other.imag_v);
-    }
-
-    ComplexImpl operator-(const ComplexImpl & other) const
-    {
-        return ComplexImpl(real_v - other.real_v, imag_v - other.imag_v);
-    }
-
-    ComplexImpl operator*(const ComplexImpl & other) const
-    {
-        return ComplexImpl(real_v * other.real_v - imag_v * other.imag_v, real_v * other.imag_v + imag_v * other.real_v);
-    }
-
-    ComplexImpl operator/(const T & other) const
-    {
-        return ComplexImpl(real_v / other, imag_v / other);
-    }
-
-    ComplexImpl & operator=(const ComplexImpl & other)
-    {
-        if (this != &other) // Check for self-assignment
-        {
-            real_v = other.real_v;
-            imag_v = other.imag_v;
-        }
-        return *this;
-    }
-
-    ComplexImpl & operator+=(const ComplexImpl & other)
-    {
-        real_v += other.real_v;
-        imag_v += other.imag_v;
-        return *this;
-    }
-
-    ComplexImpl & operator-=(const ComplexImpl & other)
-    {
-        real_v -= other.real_v;
-        imag_v -= other.imag_v;
-        return *this;
-    }
-
-    T real() const { return real_v; }
-    T imag() const { return imag_v; }
-    T norm() const { return real_v * real_v + imag_v * imag_v; }
-}; /* end class ComplexImpl */
-


Move ComplexImpl into mathtype folder for creating python wrapper.

j8xixo12 · 2025-01-02T16:36:53Z

cpp/modmesh/mathtype/Complex.hpp

+template <typename T, typename = std::enable_if_t<std::is_arithmetic<T>::value>>
+struct ComplexImpl
+{
+    T real_v;
+    T imag_v;
+
+    explicit ComplexImpl()
+        : ComplexImpl(0.0, 0.0)
+    {
+    }
+
+    explicit ComplexImpl(T r, T i)
+        : real_v(r)
+        , imag_v(i)
+    {
+    }
+
+    explicit ComplexImpl(const ComplexImpl & other)
+        : real_v(other.real_v)
+        , imag_v(other.imag_v)
+    {
+    }
+
+    ComplexImpl operator+(const ComplexImpl & other) const
+    {
+        return ComplexImpl(real_v + other.real_v, imag_v + other.imag_v);
+    }
+
+    ComplexImpl operator-(const ComplexImpl & other) const
+    {
+        return ComplexImpl(real_v - other.real_v, imag_v - other.imag_v);
+    }
+
+    ComplexImpl operator*(const ComplexImpl & other) const
+    {
+        return ComplexImpl(real_v * other.real_v - imag_v * other.imag_v, real_v * other.imag_v + imag_v * other.real_v);
+    }
+
+    ComplexImpl operator/(const T & other) const
+    {
+        return ComplexImpl(real_v / other, imag_v / other);
+    }
+
+    ComplexImpl & operator=(const ComplexImpl & other)
+    {
+        if (this != &other) // Check for self-assignment
+        {
+            real_v = other.real_v;
+            imag_v = other.imag_v;
+        }
+        return *this;
+    }
+
+    ComplexImpl & operator+=(const ComplexImpl & other)
+    {
+        real_v += other.real_v;
+        imag_v += other.imag_v;
+        return *this;
+    }
+
+    ComplexImpl & operator-=(const ComplexImpl & other)
+    {
+        real_v -= other.real_v;
+        imag_v -= other.imag_v;
+        return *this;
+    }
+
+    T real() const { return real_v; }
+    T imag() const { return imag_v; }
+    T norm() const { return real_v * real_v + imag_v * imag_v; }
+}; /* end class ComplexImpl */


Meanwhile, I'm trying to integrate with SimpleArray and create an array type Python wrapper and make it compatible with the NumPy buffer protocol, but I got a static assert

modmesh/cpp/modmesh/python/common.hpp

Line 63 in c9594da

return pybind11::detail::npy_format_descriptor<T>::dtype().is(arr.dtype());

Which is that ComplexImpl<T> is not a POD type. Currently, I don't have a firm plan to move forward.

The ending mark should be /* end structure ComplexImpl */.

ComplexImpl<T> looks like POD. What does it mean to say t is not a POD type?

j8xixo12 · 2025-01-02T16:40:04Z

cpp/modmesh/mathtype/pymod/wrap_Complex.cpp

+            .def("__complex__",
+                 [](wrapped_type const & self)
+                 { return std::complex<T>(self.real(), self.imag()); });


Making it compatible with python built-in complex type and cmath function.

j8xixo12 · 2025-01-02T16:42:01Z

@yungyuc This PR is ready for review!

yungyuc

Good move. I would like to have tightened structure of the code:

cpp/modmesh/math.hpp: Use cpp/modmesh/math/ to conform to the existing name cpp/modmesh/math.hpp. Do not create another directory name (mathtype).
test_mathtype.py: Random number is not necessary. Don't use it.

yungyuc · 2025-01-03T01:13:22Z

tests/test_mathtype.py

+
+import modmesh as mm
+
+from random import random


This is in stdlib and should be grouped with import unittest at L27.

yungyuc · 2025-01-03T01:17:36Z

tests/test_mathtype.py

+class ComplexTC(unittest.TestCase):
+
+    def setUp(self):
+        self.real1_32 = np.float32(random())


Please avoid random number in unit tests unless there is not another way. Using random number introduces a hidden dependency. If random number goes wrong, the tests here will issue false alarm. The false alarm is noise that burdens maintenance.

The random number is not printed in code and maintainers need to print it themselves when troubleshooting. This is another burden.

In this case, there are ways other than generating random number here, so random number should not be used.

You can randomly generate fixture and print the number here.

yungyuc · 2025-01-03T01:20:26Z

cpp/modmesh/math.hpp

You should move cpp/modmesh/math.hpp to cpp/modmesh/math/math.hpp. The directory should be named as cpp/modmesh/math/, not cpp/modmesh/mathtype/.

yungyuc · 2025-01-03T01:22:16Z

tests/test_mathtype.py

After changing the new directory to be cpp/modmesh/math/, this should be renamed as test_math.py.

yungyuc · 2025-01-03T01:22:56Z

modmesh/core.py

@@ -97,6 +97,8 @@
    'WorldFp32',
    'WorldFp64',
    'testhelper',
+    'ComplexFloat32',


The two lines should be placed before SimpleArray because they are more fundamental.

yungyuc · 2025-01-03T01:23:35Z

cpp/modmesh/mathtype/mathtype.hpp

This should be replaced by moving cpp/modmesh/math.hpp here.

yungyuc · 2025-01-03T01:25:33Z

cpp/modmesh/mathtype/Complex.hpp

+template <typename T, typename = std::enable_if_t<std::is_arithmetic<T>::value>>
+struct ComplexImpl
+{
+    T real_v;
+    T imag_v;
+
+    explicit ComplexImpl()
+        : ComplexImpl(0.0, 0.0)
+    {
+    }
+
+    explicit ComplexImpl(T r, T i)
+        : real_v(r)
+        , imag_v(i)
+    {
+    }
+
+    explicit ComplexImpl(const ComplexImpl & other)
+        : real_v(other.real_v)
+        , imag_v(other.imag_v)
+    {
+    }
+
+    ComplexImpl operator+(const ComplexImpl & other) const
+    {
+        return ComplexImpl(real_v + other.real_v, imag_v + other.imag_v);
+    }
+
+    ComplexImpl operator-(const ComplexImpl & other) const
+    {
+        return ComplexImpl(real_v - other.real_v, imag_v - other.imag_v);
+    }
+
+    ComplexImpl operator*(const ComplexImpl & other) const
+    {
+        return ComplexImpl(real_v * other.real_v - imag_v * other.imag_v, real_v * other.imag_v + imag_v * other.real_v);
+    }
+
+    ComplexImpl operator/(const T & other) const
+    {
+        return ComplexImpl(real_v / other, imag_v / other);
+    }
+
+    ComplexImpl & operator=(const ComplexImpl & other)
+    {
+        if (this != &other) // Check for self-assignment
+        {
+            real_v = other.real_v;
+            imag_v = other.imag_v;
+        }
+        return *this;
+    }
+
+    ComplexImpl & operator+=(const ComplexImpl & other)
+    {
+        real_v += other.real_v;
+        imag_v += other.imag_v;
+        return *this;
+    }
+
+    ComplexImpl & operator-=(const ComplexImpl & other)
+    {
+        real_v -= other.real_v;
+        imag_v -= other.imag_v;
+        return *this;
+    }
+
+    T real() const { return real_v; }
+    T imag() const { return imag_v; }
+    T norm() const { return real_v * real_v + imag_v * imag_v; }
+}; /* end class ComplexImpl */


The ending mark should be /* end structure ComplexImpl */.

yungyuc · 2025-01-03T01:26:20Z

cpp/modmesh/mathtype/Complex.hpp

+template <typename T, typename = std::enable_if_t<std::is_arithmetic<T>::value>>
+struct ComplexImpl
+{
+    T real_v;
+    T imag_v;
+
+    explicit ComplexImpl()
+        : ComplexImpl(0.0, 0.0)
+    {
+    }
+
+    explicit ComplexImpl(T r, T i)
+        : real_v(r)
+        , imag_v(i)
+    {
+    }
+
+    explicit ComplexImpl(const ComplexImpl & other)
+        : real_v(other.real_v)
+        , imag_v(other.imag_v)
+    {
+    }
+
+    ComplexImpl operator+(const ComplexImpl & other) const
+    {
+        return ComplexImpl(real_v + other.real_v, imag_v + other.imag_v);
+    }
+
+    ComplexImpl operator-(const ComplexImpl & other) const
+    {
+        return ComplexImpl(real_v - other.real_v, imag_v - other.imag_v);
+    }
+
+    ComplexImpl operator*(const ComplexImpl & other) const
+    {
+        return ComplexImpl(real_v * other.real_v - imag_v * other.imag_v, real_v * other.imag_v + imag_v * other.real_v);
+    }
+
+    ComplexImpl operator/(const T & other) const
+    {
+        return ComplexImpl(real_v / other, imag_v / other);
+    }
+
+    ComplexImpl & operator=(const ComplexImpl & other)
+    {
+        if (this != &other) // Check for self-assignment
+        {
+            real_v = other.real_v;
+            imag_v = other.imag_v;
+        }
+        return *this;
+    }
+
+    ComplexImpl & operator+=(const ComplexImpl & other)
+    {
+        real_v += other.real_v;
+        imag_v += other.imag_v;
+        return *this;
+    }
+
+    ComplexImpl & operator-=(const ComplexImpl & other)
+    {
+        real_v -= other.real_v;
+        imag_v -= other.imag_v;
+        return *this;
+    }
+
+    T real() const { return real_v; }
+    T imag() const { return imag_v; }
+    T norm() const { return real_v * real_v + imag_v * imag_v; }
+}; /* end class ComplexImpl */


ComplexImpl<T> looks like POD. What does it mean to say t is not a POD type?

j8xixo12 · 2025-01-04T11:59:28Z

@yungyuc This PR is ready for the next review.

I have made the following change:

Rename mathtype folder to math and move cpp/modmesh/math.hpp to cpp/modmesh/math
Rename test_mathtype.py to test_math.py
Using fix number instead of random number in test_math.py
Moving ComplexFloat32 and ComplexFloat64 to before SimpleArray
Fix typo in cpp/modmesh/math/Complex.hpp struct end mark

For the array type of complex number, I plan to use another PR.

yungyuc

For the array type of complex number, I plan to use another PR.

Sounds like a plan. Still having issues to be worked out:

cpp/modmesh/python/module.cpp:33: Formatting: missing blank lines after inclusion.
tests/test_math.py:39: Fixture should not use duplicate value.
Please squash. This PR does not need so many commits.

yungyuc · 2025-01-04T12:04:36Z

cpp/modmesh/python/module.cpp

@@ -30,6 +30,7 @@
 #include <modmesh/spacetime/pymod/spacetime_pymod.hpp>
 #include <modmesh/toggle/pymod/toggle_pymod.hpp>
 #include <modmesh/universe/pymod/universe_pymod.hpp>
+#include <modmesh/math/pymod/math_pymod.hpp>


Need a blank line. (This issue appears at multiple locations. Please fix them all.)

I also found that some include order do not follow the LLVM include style, for example

modmesh/cpp/modmesh/spacetime/io.hpp

Line 8 in c9594da

#include <iostream>

should I fix them ?

Thanks for spotting the inclusion order. It should not be done with the enhancement PR.

We are not strictly following inclusion order. It's not important atm, but we should optimize the inclusion at some point. If anyone cares about it, it can be done in a standalone PR.

yungyuc · 2025-01-04T12:06:55Z

tests/test_math.py

+    def setUp(self):
+        self.real1_32 = np.float32(1.0)
+        self.imag1_32 = np.float32(1.0)
+        self.real2_32 = np.float32(1.0)


real/imag 1/2 are both 1+i. Please use use different value.

j8xixo12

@yungyuc This PR is ready for the next review.

I have squashed commits, fixed missing blank lines after inclusion and using different number for test fixtures.

j8xixo12 · 2025-01-04T16:03:10Z

tests/test_math.py

+        self.real1_32 = np.float32(0.7)
+        self.imag1_32 = np.float32(1.6)
+        self.real2_32 = np.float32(2.5)
+        self.imag2_32 = np.float32(3.4)
+
+        self.real1_64 = np.float64(4.3)
+        self.imag1_64 = np.float64(5.2)
+        self.real2_64 = np.float64(6.1)
+        self.imag2_64 = np.float64(7.0)


Using different number for fixtures.

This is good.

yungyuc

Do not change blank lines in unrelated files.

Please revert all unnecessary style changes and squash.

yungyuc · 2025-01-05T01:03:04Z

cpp/modmesh/buffer/pymod/wrap_SimpleArrayPlex.cpp

@@ -29,6 +29,7 @@
 #include <modmesh/buffer/pymod/buffer_pymod.hpp> // Must be the first include.

 #include <modmesh/buffer/pymod/array_common.hpp>
+


Do not change formatting in unrelated files. Adding/removing blank lines is refactoring. Refactoring change should not be included in the same PR as feature development.

yungyuc · 2025-01-05T01:04:52Z

tests/test_math.py

+        self.real1_32 = np.float32(0.7)
+        self.imag1_32 = np.float32(1.6)
+        self.real2_32 = np.float32(2.5)
+        self.imag2_32 = np.float32(3.4)
+
+        self.real1_64 = np.float64(4.3)
+        self.imag1_64 = np.float64(5.2)
+        self.real2_64 = np.float64(6.1)
+        self.imag2_64 = np.float64(7.0)


This is good.

tigercosmos · 2025-01-05T10:21:38Z

Do you have any plan to support complex array?

yungyuc · 2025-01-05T11:47:58Z

Do you have any plan to support complex array?

It has been discussed and the conclusion is to handle it in another PR.

[...]
For the array type of complex number, I plan to use another PR.

j8xixo12 · 2025-01-05T16:16:00Z

@yungyuc This PR is ready for the next review, I have reverted all unnecessary style changes and only applied style change within related files.

yungyuc

It now looks good. Thanks for the work.

yungyuc · 2025-01-06T15:34:03Z

modmesh/core.py

@@ -96,7 +98,7 @@
    'Bezier3dFp64',
    'WorldFp32',
    'WorldFp64',
-    'testhelper',
+    'testhelper'


There should be a comma after each line in a Python list. Please be careful about it next time.

Thanks, I will be careful about this next time.

j8xixo12 commented Jan 2, 2025

View reviewed changes

yungyuc requested changes Jan 3, 2025

View reviewed changes

yungyuc assigned j8xixo12 Jan 4, 2025

yungyuc added array Multi-dimensional array implementation enhancement New feature or request labels Jan 4, 2025

yungyuc requested changes Jan 4, 2025

View reviewed changes

j8xixo12 force-pushed the complex_py_wrap branch from 20a98f0 to f97e2aa Compare January 4, 2025 16:01

j8xixo12 commented Jan 4, 2025

View reviewed changes

yungyuc requested changes Jan 5, 2025

View reviewed changes

add python complex interface

b6042a6

j8xixo12 force-pushed the complex_py_wrap branch from f97e2aa to b6042a6 Compare January 5, 2025 16:11

yungyuc approved these changes Jan 6, 2025

View reviewed changes

yungyuc merged commit de6f173 into solvcon:master Jan 6, 2025
12 checks passed

yungyuc linked an issue Jan 6, 2025 that may be closed by this pull request

Prototype for Fourier transform #454

Open

yungyuc mentioned this pull request Jan 6, 2025

Prototype for Fourier transform #454

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Complex py wrap #452

Complex py wrap #452

j8xixo12 commented Jan 2, 2025 •

edited

Loading

j8xixo12 Jan 2, 2025

j8xixo12 Jan 2, 2025

yungyuc Jan 3, 2025

yungyuc Jan 3, 2025

j8xixo12 Jan 2, 2025

j8xixo12 commented Jan 2, 2025

yungyuc left a comment •

edited

Loading

yungyuc Jan 3, 2025

yungyuc Jan 3, 2025

yungyuc Jan 3, 2025

yungyuc Jan 3, 2025

yungyuc Jan 3, 2025

yungyuc Jan 3, 2025

yungyuc Jan 3, 2025

yungyuc Jan 3, 2025

j8xixo12 commented Jan 4, 2025 •

edited

Loading

yungyuc left a comment •

edited

Loading

yungyuc Jan 4, 2025

j8xixo12 Jan 4, 2025

yungyuc Jan 4, 2025

yungyuc Jan 4, 2025

j8xixo12 left a comment

j8xixo12 Jan 4, 2025

yungyuc Jan 5, 2025

yungyuc left a comment

yungyuc Jan 5, 2025

yungyuc Jan 5, 2025

tigercosmos commented Jan 5, 2025

yungyuc commented Jan 5, 2025 •

edited

Loading

j8xixo12 commented Jan 5, 2025

yungyuc left a comment

yungyuc Jan 6, 2025

j8xixo12 Jan 8, 2025

		@@ -29,6 +29,7 @@
		#include <modmesh/buffer/pymod/buffer_pymod.hpp> // Must be the first include.

		#include <modmesh/buffer/pymod/array_common.hpp>

Complex py wrap #452

Complex py wrap #452

Conversation

j8xixo12 commented Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j8xixo12 commented Jan 2, 2025

yungyuc left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j8xixo12 commented Jan 4, 2025 • edited Loading

yungyuc left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j8xixo12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yungyuc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigercosmos commented Jan 5, 2025

yungyuc commented Jan 5, 2025 • edited Loading

j8xixo12 commented Jan 5, 2025

yungyuc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j8xixo12 commented Jan 2, 2025 •

edited

Loading

yungyuc left a comment •

edited

Loading

j8xixo12 commented Jan 4, 2025 •

edited

Loading

yungyuc left a comment •

edited

Loading

yungyuc commented Jan 5, 2025 •

edited

Loading