Fixing sym method#196
Conversation
|
Thanks for opening a Pull Request. If you want to perform a review write a comment saying: @ansys-reviewer-bot review |
|
@ansys-reviewer-bot review |
|
Okay, I will trigger a review of your PR. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #196 +/- ##
==========================================
+ Coverage 92.66% 92.84% +0.18%
==========================================
Files 2 2
Lines 586 587 +1
==========================================
+ Hits 543 545 +2
+ Misses 43 42 -1 |
|
@ansys-reviewer-bot review |
|
Okay, I will trigger a review of your PR. |
|
@ansys-reviewer-bot review |
|
Okay, I will trigger a review of your PR. |
![ansys-reviewer-bot[bot]](https://avatars.githubusercontent.com/in/383621?s=60&v=4){kind=small}
| if sym is None: | ||
| if linalg.issymmetric(arr): | ||
| sym = True | ||
| else: | ||
| sym = False |
There was a problem hiding this comment.
The issymmetric function from linalg expects a NumPy array, not a sparse matrix. The condition might not work as intended with a sparse matrix. You can convert the sparse matrix to a dense one using arr.toarray() and then use issymmetric or implement a custom approach to check symmetry on the sparse matrix.
| if mapdl_version < 23.2: # pragma: no cover | ||
| raise VersionError("``kron`` requires MAPDL version 2023R2") | ||
|
|
||
| if not isinstance(obj, AnsMath): | ||
| if not isinstance(obj, AnsMathObj): |
There was a problem hiding this comment.
To improve the error message, you could include the object type in the description:
| if mapdl_version < 23.2: # pragma: no cover | |
| raise VersionError("``kron`` requires MAPDL version 2023R2") | |
| if not isinstance(obj, AnsMath): | |
| if not isinstance(obj, AnsMathObj): | |
| if not isinstance(obj, AnsMathObj): | |
| raise TypeError(f"Must be an AnsMath object, but got {type(obj)}.") |
| @@ -1,7 +1,9 @@ | |||
| import os | |||
There was a problem hiding this comment.
Overall, the patch looks good. A few suggestions and comments will be provided below.
|
|
||
| @pytest.fixture | ||
| def sparse_asym_mat(): | ||
| return sparse.random(5000, 5000, density=0.05, format="csr") |
There was a problem hiding this comment.
Consider setting a random seed for reproducibility before generating the sparse random matrix.
| @@ -182,19 +182,35 @@ | |||
| assert m1.shape == shape | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Replace the current assert message with a more informative one:
| assert sparse_mat.data.nbytes // 1024**2 > 4, f"Matrix size is not over gRPC message limit (4 MB): {sparse_mat.data.nbytes // 1024**2} MB" |
| def test_kron_product_unsupported_dtype(mm): | ||
| mapdl_version = mm._mapdl.version | ||
| if mapdl_version < 23.2: | ||
| pytest.skip("Requires MAPDL 2023 R2 or later.") |
There was a problem hiding this comment.
Replace the current assert message with a more informative one:
| pytest.skip("Requires MAPDL 2023 R2 or later.") | |
| with pytest.raises(TypeError, match=r"Must be an AnsMath object. not a local object"): |
|
|
||
| def test_sym_dmat(mm, dense_sym_mat): | ||
| dmat = mm.matrix(dense_sym_mat) | ||
| if not server_meets_version(mm._server_version, (0, 5, 0)): |
There was a problem hiding this comment.
Nice addition of tests for symmetric dense and sparse matrices. It helps check if the server version meets the requirements.
Please make the suggested changes to the assert messages for better readability and submit the patch for review again.
|
Symmetric matrices are stored with upper triangular ones within MAPDL. storage_matrix = [[x x x x],
[. x x x],
[. . x x],
[. . . x]]
symmetric_matrix = storage_matrix + storage_matrix.T - diag(storage_matrix) |
|
Hello @clatapie! If this issue needs to remain open, please comment below with If you want this repository to be excluded from this automated maintenance process, please let us know by filling in the opt-out request form. |
4 participants
Closes #195.