Refactor: refactor the constructor of SimpleArray #439

ThreeMonth03 · 2024-12-04T18:33:10Z

In the pull request #438, @yungyuc mentioned that I could refactor the constructor.

ThreeMonth03

@yungyuc @tigercosmos Could you please review this pull request?

ThreeMonth03 · 2024-12-04T18:41:48Z

cpp/modmesh/buffer/SimpleArray.hpp

-                         bool is_c_contiguous = true,
-                         bool is_f_contiguous = false)
+                         bool c_contiguous,
+                         bool f_contiguous)
+        : SimpleArray(shape, stride, buffer)


In order to avoid

error: call to constructor of 'SimpleArray<bool>' is ambiguous 330 | return SimpleArray(shape, stride, buffer);

I don't assign the default value to c_contiguous and f_contiguous.

Since we only have C type or F type, why not just:

explicit SimpleArray(small_vector<size_t> const & shape, small_vector<size_t> const & stride, std::shared_ptr<buffer_type> const & buffer, bool is_c_contiguous = true)

I prefer using enum more, just for your reference:

enum class SimpleArrayMemoryType { CType FType }; explicit SimpleArray(small_vector<size_t> const & shape, small_vector<size_t> const & stride, std::shared_ptr<buffer_type> const & buffer, SimpleArrayMemoryType is_c_contiguous = SimpleArrayMemoryType::CType)

Boolean flag does not need to introduce new symbols at the Python side and is preferred ATM.

The input buffer may be F or C contiguous or discontiguous. A single Boolean flag is not enough.

And it's good to remove default arguments in C++. They usually do harm more than good.

On the Python side, the argument can be a string; we already have the example from DataType.

It can, but using a string for a C++ enum is not the most robust implementation. It can be tolerated if the API is necessary. We do not need the enum so it's not the case here.

By the way, if the array is 1 dimension, it could be c-contiguous and f-contiguous simultaneously, and I'm not sure enum is a good manner.

By the way, if the array is 1 dimension, it could be c-contiguous and f-contiguous simultaneously, and I'm not sure enum is a good manner.

Good point. I overlooked the 1D case. But 1D should be a simple case. We can leave it here and revisit for a more complete solution in the future.

ThreeMonth03 · 2024-12-04T18:51:08Z

cpp/modmesh/buffer/SimpleArray.hpp

    explicit SimpleArray(small_vector<size_t> const & shape,
                         small_vector<size_t> const & stride,
                         std::shared_ptr<buffer_type> const & buffer,
-                         bool is_c_contiguous = true,
-                         bool is_f_contiguous = false)
+                         bool c_contiguous,
+                         bool f_contiguous)
+        : SimpleArray(shape, stride, buffer)
+    {
+        if (shape.size() != stride.size())
+        {
+            throw std::runtime_error("SimpleArray: shape and stride size mismatch");
+        }
+        if (c_contiguous)
+        {
+            check_c_contiguous(shape, stride);
+        }
+        if (f_contiguous)
+        {
+            check_f_contiguous(shape, stride);
+        }
+    }


@jteng2127 suggest that I could create a constructor with c_contiguous and f_contiguous, and this constructor could check the contiguous property and call another construcotr without c_contiguous and f_contiguous.

ThreeMonth03 · 2024-12-04T18:54:27Z

cpp/modmesh/buffer/pymod/wrap_SimpleArray.cpp

-                        const bool is_c_contiguous = (arr_in.flags() & py::array::c_style) == py::array::c_style;
-                        const bool is_f_contiguous = (arr_in.flags() & py::array::f_style) == py::array::f_style;
+                        auto && is_c_contiguous = [&]() -> bool
+                        {
+                            return (arr_in.flags() & py::array::c_style) == py::array::c_style;
+                        };
+                        auto && is_f_contiguous = [&]() -> bool
+                        {
+                            return (arr_in.flags() & py::array::f_style) == py::array::f_style;
+                        };


I'm not sure whether wrapping with lambda function is better than initializing the boolean variable.

These lambda implementations work the same as direct evaluation. They do not simplify anything either. Please revert them.

yungyuc

Good work! One thing to revert:

wrap_SimpleArray.cpp:87: Revert the unnecessary lambdas.

yungyuc · 2024-12-05T05:41:52Z

cpp/modmesh/buffer/SimpleArray.hpp

-                         bool is_c_contiguous = true,
-                         bool is_f_contiguous = false)
+                         bool c_contiguous,
+                         bool f_contiguous)
+        : SimpleArray(shape, stride, buffer)


And it's good to remove default arguments in C++. They usually do harm more than good.

yungyuc · 2024-12-05T05:44:37Z

cpp/modmesh/buffer/pymod/wrap_SimpleArray.cpp

-                        const bool is_c_contiguous = (arr_in.flags() & py::array::c_style) == py::array::c_style;
-                        const bool is_f_contiguous = (arr_in.flags() & py::array::f_style) == py::array::f_style;
+                        auto && is_c_contiguous = [&]() -> bool
+                        {
+                            return (arr_in.flags() & py::array::c_style) == py::array::c_style;
+                        };
+                        auto && is_f_contiguous = [&]() -> bool
+                        {
+                            return (arr_in.flags() & py::array::f_style) == py::array::f_style;
+                        };


These lambda implementations work the same as direct evaluation. They do not simplify anything either. Please revert them.

yungyuc · 2024-12-05T05:47:46Z

cpp/modmesh/buffer/SimpleArray.hpp

@@ -623,6 +608,37 @@ class SimpleArray
    value_type * body() { return m_body; }

 private:
+    void check_c_contiguous(small_vector<size_t> const & shape,


I like the helper functions.

They do not need to be in the class template because they do not use the template argument. But it's OK to keep them here for now.

There are other member functions that can be moved outside the class template. We only need to refactor when we run out of interesting things to do.

ThreeMonth03

wrap_SimpleArray.cpp:87: Revert the unnecessary lambdas.

Fixed. @yungyuc Could you please review this pull request?

yungyuc

Perfect

yungyuc · 2024-12-07T00:59:51Z

cpp/modmesh/buffer/SimpleArray.hpp

-                         bool is_c_contiguous = true,
-                         bool is_f_contiguous = false)
+                         bool c_contiguous,
+                         bool f_contiguous)
+        : SimpleArray(shape, stride, buffer)


By the way, if the array is 1 dimension, it could be c-contiguous and f-contiguous simultaneously, and I'm not sure enum is a good manner.

Good point. I overlooked the 1D case. But 1D should be a simple case. We can leave it here and revisit for a more complete solution in the future.

ThreeMonth03 force-pushed the issue432 branch from bfb8cea to c72e55f Compare December 4, 2024 18:34

ThreeMonth03 commented Dec 4, 2024

View reviewed changes

yungyuc requested changes Dec 5, 2024

View reviewed changes

yungyuc assigned ThreeMonth03 Dec 5, 2024

yungyuc added refactor Change code without changing tests array Multi-dimensional array implementation labels Dec 5, 2024

ThreeMonth03 force-pushed the issue432 branch 2 times, most recently from 753353f to 16857e5 Compare December 6, 2024 19:03

ThreeMonth03 commented Dec 6, 2024

View reviewed changes

Refactor: refactor the constructor of SimpleArray

74a3a0f

ThreeMonth03 force-pushed the issue432 branch from 16857e5 to 74a3a0f Compare December 6, 2024 19:21

yungyuc approved these changes Dec 7, 2024

View reviewed changes

yungyuc merged commit 2e43e1f into solvcon:master Dec 7, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: refactor the constructor of SimpleArray #439

Refactor: refactor the constructor of SimpleArray #439

ThreeMonth03 commented Dec 4, 2024

ThreeMonth03 left a comment

ThreeMonth03 Dec 4, 2024 •

edited

Loading

tigercosmos Dec 5, 2024

yungyuc Dec 5, 2024 •

edited

Loading

yungyuc Dec 5, 2024

tigercosmos Dec 5, 2024

yungyuc Dec 5, 2024 •

edited

Loading

ThreeMonth03 Dec 6, 2024

yungyuc Dec 7, 2024

ThreeMonth03 Dec 4, 2024

ThreeMonth03 Dec 4, 2024

yungyuc Dec 5, 2024

yungyuc left a comment •

edited

Loading

yungyuc Dec 5, 2024

yungyuc Dec 5, 2024

yungyuc Dec 5, 2024

ThreeMonth03 left a comment

yungyuc left a comment

yungyuc Dec 7, 2024

Refactor: refactor the constructor of SimpleArray #439

Refactor: refactor the constructor of SimpleArray #439

Conversation

ThreeMonth03 commented Dec 4, 2024

ThreeMonth03 left a comment

Choose a reason for hiding this comment

ThreeMonth03 Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yungyuc Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yungyuc Dec 5, 2024 • 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

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

ThreeMonth03 left a 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

ThreeMonth03 Dec 4, 2024 •

edited

Loading

yungyuc Dec 5, 2024 •

edited

Loading

yungyuc Dec 5, 2024 •

edited

Loading

yungyuc left a comment •

edited

Loading