-
Notifications
You must be signed in to change notification settings - Fork 56
Implement basic 2d SimpleArray operation #618
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
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 You could review this pull request. Thanks.
| template <typename A, typename T> | ||
| class SimpleArrayMixinMatrix | ||
| { | ||
|
|
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.
They are 2d operations, so I name the class SimpleArrayMixinMatrix. How do you think?
| A symmetrize() const | ||
| { | ||
| auto athis = static_cast<A const *>(this); | ||
| validate_square("symmetrize"); | ||
| A result = *athis; | ||
| if constexpr (is_complex_v<value_type>) | ||
| { | ||
| for (ssize_t i = 0; i < athis->shape(0); ++i) | ||
| { | ||
| for (ssize_t j = 0; j < athis->shape(1); ++j) | ||
| { | ||
| result(i, j) = (result(i, j) + (*athis)(j, i).conj()) / static_cast<value_type>(2.0); | ||
| } | ||
| } | ||
| } | ||
| else | ||
| { | ||
| for (ssize_t i = 0; i < athis->shape(0); ++i) | ||
| { | ||
| for (ssize_t j = 0; j < athis->shape(1); ++j) | ||
| { | ||
| result(i, j) = (result(i, j) + (*athis)(j, i)) / static_cast<value_type>(2.0); | ||
| } | ||
| } | ||
| } | ||
| return result; | ||
| } |
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 used the following functions, but I noticed that the add function doesn't support the non-contiguous SimpleArray.
result.hermitian().add(*athis).mul(0.5);Therefore, I implement the function without calling the function add().
| static array_type create_noise_matrix(size_t size, real_type noise_var) | ||
| { | ||
| return array_type::eye(size).mul(static_cast<T>(noise_var)); | ||
| } | ||
| void check_dimensions(); | ||
| void check_measurement(array_type const & z); | ||
| void check_control(array_type const & u); | ||
| static array_type symmetrize(array_type const & p); | ||
| static array_type hermitian(array_type const & a); |
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 these helper functions.
| }; /* end class SimpleArrayMixinSearch */ | ||
|
|
||
| template <typename A, typename T> | ||
| class SimpleArrayMixinMatrix |
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 feel like we need to have a well-defined matrix class 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.
It is impossible to define a matrix class well at the moment. Doing that now is over-engineering.
We will do it later.
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.
Point to address:
- In testing, add code to check the error messages for wrong shape. (
eyedoes not have it.)
| }; /* end class SimpleArrayMixinSearch */ | ||
|
|
||
| template <typename A, typename T> | ||
| class SimpleArrayMixinMatrix |
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.
It is impossible to define a matrix class well at the moment. Doing that now is over-engineering.
We will do it later.
| ): | ||
| sarr2.idiv(sarr1) | ||
|
|
||
| def test_eye(self): |
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.
Also test for wrong shape input.
| self.assertEqual(shermitian.shape, ndhermitian.shape) | ||
| np.testing.assert_equal(shermitian.ndarray, ndhermitian) | ||
|
|
||
| def test_hermitian_error(self): |
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.
Consider to merge the error checking into the main test function as a section. It looks like over-engineering to split at this early stage.
|
@ThreeMonth03 any update? |
Sorry, not yet. I was a bit lost with my new job last week and spent most of the time reading the company’s code. |
According the discussion in issue #601 and pr #607, I move some operations from kalman filter to SimpleArray, including: