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.

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 unit test for SimpleArray #445

Merged
merged 1 commit into from
Jan 1, 2025

Conversation

KHLee529
Copy link
Contributor

This is a follow up of PR #444 that test the constructor of SimpleArray.

@yungyuc yungyuc added test testing and continuous integration array Multi-dimensional array implementation labels Dec 26, 2024
@yungyuc
Copy link
Member

yungyuc commented Dec 26, 2024

Please review the CI failures. And please make sure all tests are clean before filing a PR.

@yungyuc
Copy link
Member

yungyuc commented Dec 26, 2024

@ThreeMonth03 The profiler test is not stable. When running it locally I can feel its slowness too. Those are 2 issues. Could you please help take a look at the first and we find time to discuss for the second.

GHA log https://github.com/solvcon/modmesh/actions/runs/12493102514/job/34861154319?pr=445#step:10:348 :

[----------] 4 tests from CallProfilerTest
[ RUN      ] CallProfilerTest.test_print_result
[       OK ] CallProfilerTest.test_print_result (61 ms)
[ RUN      ] CallProfilerTest.test_simple_case1
[       OK ] CallProfilerTest.test_simple_case1 (61 ms)
[ RUN      ] CallProfilerTest.simple_case_2
make[4]: *** [gtests/CMakeFiles/run_gtest] Error 1
/Users/runner/work/modmesh/modmesh/gtests/test_nopython_callprofiler.cpp:154: Failure
make[3]: *** [gtests/CMakeFiles/run_gtest.dir/all] Error 2
Value of: diff_time(node1->data().total_time, uniqueTime1 + uniqueTime2 + uniqueTime3)
make[2]: *** [gtests/CMakeFiles/run_gtest.dir/rule] Error 2
  Actual: false
make[1]: *** [run_gtest] Error 2
Expected: true
make: *** [gtest] Error 2
/Users/runner/work/modmesh/modmesh/gtests/test_nopython_callprofiler.cpp:160: Failure
Value of: diff_time(node2->data().total_time, uniqueTime1 + uniqueTime2)
  Actual: false
Expected: true
/Users/runner/work/modmesh/modmesh/gtests/test_nopython_callprofiler.cpp:166: Failure
Value of: diff_time(node3->data().total_time, uniqueTime1)
  Actual: false
Expected: true
[  FAILED  ] CallProfilerTest.simple_case_2 (166 ms)
[ RUN      ] CallProfilerTest.cancel
[       OK ] CallProfilerTest.cancel (0 ms)
[----------] 4 tests from CallProfilerTest (289 ms total)

@@ -0,0 +1,20 @@
from modmesh import SimpleArrayUint64, SimpleArrayFloat64
Copy link
Collaborator

Choose a reason for hiding this comment

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

I will prefer to put the code at test_buffer.py, where we put all other array tests.

Copy link
Member

Choose a reason for hiding this comment

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

I concur. Do not add a new test file for this simple test.

@KHLee529
Copy link
Contributor Author

@yungyuc @tigercosmos thanks for the suggestion, I'll inform you after I review and refine the code.

@KHLee529 KHLee529 force-pushed the add_simplearray_test branch from 99456c2 to 2d83fd3 Compare December 30, 2024 12:34
@KHLee529
Copy link
Contributor Author

@yungyuc I've moved the test into test_buffer.py and fixed linter error.

Copy link
Member

@yungyuc yungyuc left a comment

Choose a reason for hiding this comment

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

Please also test using the original testing code in issue #437:

import unittest
from modmesh import SimpleArrayUint64, SimpleArrayFloat64
import numpy as np

class TimeBuffer(unittest.TestCase):
    a = np.ndarray(shape= (5,5,5,5), dtype=float)
    for i in range(5):
        for j in range(5):
            for k in range(5):
                for l in range(5):
                    a[i,j,k,l] = i*1000 + j*100 + k*10 + l

    b_copy = SimpleArrayFloat64(array=a)
    print(b_copy.ndarray)

Also address the following:

  • L440: Use more interesting shape.
  • L441: Use string for dtype.

@@ -436,6 +437,11 @@ def test_SimpleArray_from_ndarray(self):
sarr_from_cpp = modmesh.SimpleArrayFloat64(shape=(2, 3, 4))
self.assertFalse(sarr_from_cpp.is_from_python)

shape = (2, 2, 2, 2)
np_sarr = np.ndarray(shape=shape, dtype=np.float64)
Copy link
Member

Choose a reason for hiding this comment

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

Use string for the argument dtype, i.e., write dtype='float64'. modmesh uses a convention to use string for numpy dtype argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK I'll fix it

@@ -436,6 +437,11 @@ def test_SimpleArray_from_ndarray(self):
sarr_from_cpp = modmesh.SimpleArrayFloat64(shape=(2, 3, 4))
self.assertFalse(sarr_from_cpp.is_from_python)

shape = (2, 2, 2, 2)
Copy link
Member

Choose a reason for hiding this comment

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

Could you please also test with the shape in which each element different from others? (2, 2, 2, 2) is too simple.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem, I'll make it to (2, 3, 5, 7).
Here, I'm somehow curious about how to measure how "interesting" the shape is. I don't really know how to make the number reasonable but still effective as a test case. For example, (111, 222, 333, 444) might be an unreasonable test case due to its huge memory consumption.

Copy link
Member

Choose a reason for hiding this comment

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

(2, 3, 5, 7) looks sufficiently interesting.

@yungyuc
Copy link
Member

yungyuc commented Dec 30, 2024

Please leave a global comment every time when you are ready for requesting the next review.

@KHLee529
Copy link
Contributor Author

@yungyuc Oh, thanks for reminding. Now is ready for next review!

@yungyuc yungyuc force-pushed the add_simplearray_test branch from 0c95076 to e3a4812 Compare January 1, 2025 03:18
Copy link
Member

@yungyuc yungyuc left a comment

Choose a reason for hiding this comment

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

I make some tweaks and rebase to the latest master.

@@ -436,6 +437,22 @@ def test_SimpleArray_from_ndarray(self):
sarr_from_cpp = modmesh.SimpleArrayFloat64(shape=(2, 3, 4))
self.assertFalse(sarr_from_cpp.is_from_python)

shape = (2, 3, 5, 7)
np_sarr = np.array(shape=shape, dtype='float64')
Copy link
Member

Choose a reason for hiding this comment

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

Use np.array instead of np.ndarray. The latter is not a numpy coding convention. We do not usually directly call the ndarray constructor. Instead, we use the helpers array, empty, ones, zeros, full, etc.

@yungyuc yungyuc force-pushed the add_simplearray_test branch from e3a4812 to ea79063 Compare January 1, 2025 03:27
@yungyuc yungyuc force-pushed the add_simplearray_test branch from ea79063 to 6962dcf Compare January 1, 2025 03:39
Copy link
Member

@yungyuc yungyuc left a comment

Choose a reason for hiding this comment

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

The testing code looks good. I squashed all changes to prepare for merging.

@yungyuc yungyuc merged commit 8f7ad03 into solvcon:master Jan 1, 2025
12 checks passed
@KHLee529 KHLee529 deleted the add_simplearray_test branch January 1, 2025 04:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
array Multi-dimensional array implementation test testing and continuous integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Incorrect construction of SimpleArray from a high-dimension ndarray
3 participants