Add `hermitian()` to matrix gallery #1199

ClaudiaComito · 2023-08-19T05:51:12Z

Due Diligence

General:
- base branch must be main for new features, latest release branch (e.g. release/1.3.x) for bug fixes
- title of the PR is suitable to appear in the Release Notes
Implementation:
- unit tests: all split configurations tested
- unit tests: multiple dtypes tested
- documentation updated where needed

Description

Creating a new PR to add tests for the Hermitian test matrix implemented in #1086.

Workflow:

merge Add hermitian() to matrix gallery #1086 into this PR ✅
merge this PR into main

Issue/s resolved: #1073

Changes proposed:

Type of change

Memory requirements

Performance

Does this change modify the behaviour of other functions? If so, which?

no

ghost · 2023-08-19T05:52:55Z

👇 Click on the image for a new way to code review

Legend

* hermitian: add function definition * implement hermitian * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Set default dtype to complex * Edit docs * Apply suggestions from code review * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix reference to types --------- Co-authored-by: Claudia Comito <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

github-actions · 2023-08-19T06:04:29Z

Thank you for the PR!

github-actions · 2023-08-19T06:10:39Z

Thank you for the PR!

github-actions · 2023-08-19T06:16:38Z

Thank you for the PR!

codecov · 2023-08-19T06:27:05Z

Codecov Report

Merging #1199 (833522d) into main (089ab2f) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head 833522d differs from pull request most recent head 08d3dca. Consider uploading reports for the commit 08d3dca to get more accurate results

@@            Coverage Diff             @@
##             main    #1199      +/-   ##
==========================================
- Coverage   92.19%   92.16%   -0.03%     
==========================================
  Files          75       75              
  Lines       10699    10713      +14     
==========================================
+ Hits         9864     9874      +10     
- Misses        835      839       +4

Flag	Coverage Δ
unit	`92.16% <100.00%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
heat/utils/data/matrixgallery.py	`100.00% <100.00%> (ø)`

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

mrfh92 · 2023-08-21T11:05:39Z

heat/utils/data/matrixgallery.py

+    dtype: Type[datatype] = core.complex64,
+) -> DNDarray:
+    """
+    Generates a Hermitian matrix of size `(n,n)`. A Hermitian matrix is a  complex square matrix that is equal to its conjugate transpose.


In essence, the matrices constructed below are distributed according to "Whishart distribution" (https://en.wikipedia.org/wiki/Wishart_distribution). We could add a reference to "Marchenko-Pastur distribution" for their singular values (https://en.wikipedia.org/wiki/Marchenko%E2%80%93Pastur_distribution) if we scale appropriately.

@mrfh92 would you update the docs? Thanks a lot!

mrfh92 · 2023-08-21T11:14:23Z

heat/utils/data/matrixgallery.py

+    real_dtype = core.float32 if dtype is core.complex64 else core.float64
+    matrix = randn(n, n, dtype=real_dtype, split=split, device=device)
+    matrix += 1j * randn(n, n, dtype=real_dtype, split=split, device=device)
+    return matrix @ core.conj(matrix).T


Two comments:

if we scale by 1/matrix.shape[0] the singular values are distributed according to Marchenko Pastur distribution (see above)... also this scaling might remove unwanted effects when choosing larger and larger matrices

The formula yields, of course, and they are even positive definite (with probability 1? - Im not an expert in this stuff...). However, doing the matrix-multiplication is quite expensive ... can we add something like an option "positive_definite" that triggers the formula used so far and use

return matrix + core.conj(matrix).T.resplit(matrix.split)

instead for the remaining cases?

Thanks, done

mrfh92

only small comments on docs and formula used for generation of the random matrices.
But in prinicple, this PR is ready for merging even without these changes.

Thanks for the work 👍

Updated docs for `hermitian` Added scaling factor for the positive definite matrices Added distinction of real and complex data types

for more information, see https://pre-commit.ci

extended tests: * added tests for the positive definite case and real data-types

for more information, see https://pre-commit.ci

…ine with latex formulas...

github-actions · 2023-08-25T09:23:41Z

Thank you for the PR!

github-actions · 2023-08-29T11:38:32Z

Thank you for the PR!

mrfh92 · 2023-08-31T14:08:06Z

@ClaudiaComito since I have done some changes here last week after my review it might be more useful if now you review again
(sry I just see that I cant assign you to be reviewer for your own PR)

mrfh92

Looks fine to me, but since the last changes have been done by me, maybe somebody else should review too.

add test_hermitian()

87c2d52

ClaudiaComito changed the title ~~Add Hermitian test matrix~~ Add hermitian() to matrix gallery Aug 19, 2023

ClaudiaComito added 2 commits August 19, 2023 08:04

fix operand split for binary operation

78bc754

add double precision tests

833522d

ClaudiaComito requested a review from mrfh92 August 19, 2023 06:11

ClaudiaComito added the linalg label Aug 19, 2023

ClaudiaComito added this to the Repo Clean-Up milestone Aug 19, 2023

mrfh92 reviewed Aug 21, 2023

View reviewed changes

faster solution for non-positive definite hermitian

1c65761

ClaudiaComito added the PR talk label Aug 23, 2023

mrfh92 and others added 8 commits August 25, 2023 09:37

Merge branch 'main' into 1073-Add_Hermitian_to_matrix_gallery

139b2c7

Update matrixgallery.py

a8b0ca2

Updated docs for `hermitian` Added scaling factor for the positive definite matrices Added distinction of real and complex data types

[pre-commit.ci] auto fixes from pre-commit.com hooks

3618938

for more information, see https://pre-commit.ci

Update test_matrixgallery.py

87404a7

extended tests: * added tests for the positive definite case and real data-types

[pre-commit.ci] auto fixes from pre-commit.com hooks

67e0fc6

for more information, see https://pre-commit.ci

reformatting with precommit

08d3dca

formatted the docstring as raw (r""" ...) in order to let flake8 be f…

18b212c

…ine with latex formulas...

corrected a typo in the tests...

d337509

ClaudiaComito added the merge queue label Aug 28, 2023

Merge branch 'main' into 1073-Add_Hermitian_to_matrix_gallery

e6acd58

mrfh92 requested review from mrfh92 and mtar and removed request for mrfh92 August 31, 2023 14:10

mrfh92 approved these changes Aug 31, 2023

View reviewed changes

ClaudiaComito merged commit 425eba6 into main Sep 4, 2023
41 checks passed

mtar removed the PR talk label Sep 11, 2023

mtar deleted the 1073-Add_Hermitian_to_matrix_gallery branch February 28, 2024 09:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `hermitian()` to matrix gallery #1199

Add `hermitian()` to matrix gallery #1199

ClaudiaComito commented Aug 19, 2023 •

edited

Loading

ghost commented Aug 19, 2023 •

edited by ghost

Loading

Legend

github-actions bot commented Aug 19, 2023

github-actions bot commented Aug 19, 2023

github-actions bot commented Aug 19, 2023

codecov bot commented Aug 19, 2023 •

edited

Loading

mrfh92 Aug 21, 2023 •

edited

Loading

ClaudiaComito Aug 21, 2023

mrfh92 Aug 21, 2023

ClaudiaComito Aug 21, 2023

mrfh92 left a comment

github-actions bot commented Aug 25, 2023

github-actions bot commented Aug 29, 2023

mrfh92 commented Aug 31, 2023 •

edited

Loading

mrfh92 left a comment

Add hermitian() to matrix gallery #1199

Add hermitian() to matrix gallery #1199

Conversation

ClaudiaComito commented Aug 19, 2023 • edited Loading

Due Diligence

Description

Changes proposed:

Type of change

Memory requirements

Performance

Does this change modify the behaviour of other functions? If so, which?

ghost commented Aug 19, 2023 • edited by ghost Loading

Legend

github-actions bot commented Aug 19, 2023

github-actions bot commented Aug 19, 2023

github-actions bot commented Aug 19, 2023

codecov bot commented Aug 19, 2023 • edited Loading

Codecov Report

mrfh92 Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

ClaudiaComito Aug 21, 2023

Choose a reason for hiding this comment

mrfh92 Aug 21, 2023

Choose a reason for hiding this comment

ClaudiaComito Aug 21, 2023

Choose a reason for hiding this comment

mrfh92 left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 25, 2023

github-actions bot commented Aug 29, 2023

mrfh92 commented Aug 31, 2023 • edited Loading

mrfh92 left a comment

Choose a reason for hiding this comment

Add `hermitian()` to matrix gallery #1199

Add `hermitian()` to matrix gallery #1199

ClaudiaComito commented Aug 19, 2023 •

edited

Loading

ghost commented Aug 19, 2023 •

edited by ghost

Loading

codecov bot commented Aug 19, 2023 •

edited

Loading

mrfh92 Aug 21, 2023 •

edited

Loading

mrfh92 commented Aug 31, 2023 •

edited

Loading