feat: add rand_circulant_gram func #389

ankit-pn · 2023-12-22T14:15:08Z

Description

~~add feat described in~~ Fixes #379

Status

Ready to go

codecov · 2023-12-22T14:19:20Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (cbd1575) 97.6% compared to head (2ac70b4) 98.0%.
Report is 29 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #389     +/-   ##
========================================
+ Coverage    97.6%   98.0%   +0.4%     
========================================
  Files         154     156      +2     
  Lines        3059    3065      +6     
  Branches      744     742      -2     
========================================
+ Hits         2987    3006     +19     
+ Misses         45      37      -8     
+ Partials       27      22      -5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

purva-thakre · 2023-12-26T02:14:25Z

toqito/rand/random_circulant_gram.py

+def random_circulant_gram(dim: int) -> np.ndarray:
+    r"""Generate a random circulant Gram matrix of specified dimension.
+
+    This function creates a circulant matrix based on a random diagonal matrix and the normalized Discrete


It might be better to add some details/ref on what a circulant matrix is in this docstring + more details on what a normalized DFT is.

https://en.wikipedia.org/wiki/Circulant_matrix
https://ccrma.stanford.edu/~jos/st/Normalized_DFT.html

I have made the changes, please review it and let me know if further modification is required.

Might want to double-check how references are added. Other docstrings might be helpful for you.

@ankit-pn I think you'll want to add the links you provided within the references in docs/articles.bib. So for instance,

@misc{WikiCirculantMat, author = "Wikipedia", title = "Circulant matrix", howpublished = {https://en.wikipedia.org/wiki/Circulant_matrix} }

And then use the :cite: command in the docstring (as done elsewhere in toqito) instead of putting the whole link there. Similarly for your other link as well.

ankit-pn · 2023-12-27T02:25:06Z

I have done the changes that you have recommended. Please review it.
Thanks.

vprusso · 2023-12-28T13:09:30Z

toqito/rand/random_circulant_gram.py

+def random_circulant_gram(dim: int) -> np.ndarray:
+    r"""Generate a random circulant Gram matrix of specified dimension.
+
+    This function creates a circulant matrix based on a random diagonal matrix and the normalized Discrete


@ankit-pn I think you'll want to add the links you provided within the references in docs/articles.bib. So for instance,

@misc{WikiCirculantMat, author = "Wikipedia", title = "Circulant matrix", howpublished = {https://en.wikipedia.org/wiki/Circulant_matrix} }

And then use the :cite: command in the docstring (as done elsewhere in toqito) instead of putting the whole link there. Similarly for your other link as well.

vprusso · 2023-12-28T13:11:39Z

toqito/rand/random_circulant_gram.py

+def random_circulant_gram(dim: int) -> np.ndarray:
+    r"""Generate a random circulant Gram matrix of specified dimension.
+
+    A circulant matrix is a square matrix where each row is a shifted version of the one above,


Something seems a bit off with the formatting of these lines. They should have a length of 120, but these look like they are around 80.

Probably because black was used for formatting issues?

yes, it was because i was using formatter in wrong way. i have corrected it and do let me know if any other further changes is required.

vprusso · 2023-12-28T13:12:44Z

toqito/rand/tests/test_random_circulant_gram.py

+    circulant_matrix = random_circulant_gram(dim)
+
+    # Ensure the matrix has the correct shape.
+    assert circulant_matrix.shape == (dim, dim)


Instead of using assert here we should be using np.testing.assert_equal (check out the other tests for reference)

purva-thakre · 2023-12-29T00:20:18Z

docs/rand.rst

@@ -17,3 +17,4 @@ Random
    toqito.rand.random_state_vector
    toqito.rand.random_states
    toqito.rand.random_unitary
+    toqito.rand.random_circulant_gram


minor nit: might want to move this addition up (alphabetical order)

toqito/rand/__init__.py

toqito/rand/random_circulant_gram.py

Co-authored-by: Purva Thakre <66048318+purva-thakre@users.noreply.github.com>

purva-thakre · 2023-12-29T14:22:43Z

toqito/rand/random_circulant_gram.py

+    This function utilizes the normalized DFT, a variation of DFT with normalized basis vectors. This
+    variation alters computational requirements and offers a different view on signal transformations.
+
+    :cite:`StanfordNormDFT`.
+
+    The function creates a circulant matrix from a random diagonal matrix and the normalized DFT matrix.
+    First, it generates a diagonal matrix with random non-negative entries. Next, it constructs the
+    normalized DFT matrix. Finally, it computes the circulant matrix, which is real due to its origin
+    from the DFT of a real diagonal matrix.


It might be just me, but this part seems vague: This variation alters computational requirements and offers a different view on signal transformations

Could this be re-worded differently?

toqito/rand/random_circulant_gram.py

Co-authored-by: Purva Thakre <66048318+purva-thakre@users.noreply.github.com>

ankit-pn · 2023-12-29T14:36:43Z

@purva-thakre i have pushed the changes, error was because of ruff check. do let me know know if any further changes required. thanks.

toqito/rand/random_circulant_gram.py

Co-authored-by: Purva Thakre <66048318+purva-thakre@users.noreply.github.com>

vprusso · 2024-01-07T13:34:04Z

I think this is good to go, nice job, @ankit-pn !

Anything additional to address before merging, @purva-thakre ?

purva-thakre · 2024-01-07T21:30:34Z

I think this is good to go, nice job, @ankit-pn !

Anything additional to address before merging, @purva-thakre ?

I think one of the lines in the docstring is vague. I don't think it is related to what the function is intended to do. #389 (comment)

But besides this, LGTM.

toqito/rand/random_circulant_gram.py

Co-authored-by: Purva Thakre <66048318+purva-thakre@users.noreply.github.com>

vprusso · 2024-01-10T19:36:59Z

Thanks for pushing this over the line, @purva-thakre

And thank you as well to @ankit-pn for putting this feature into toqito!

Fixing linter.

Removing whitespace.

ankit-pn added 3 commits December 22, 2023 19:40

feat: add random circulant gram func

244d50e

fix: add random_circulant_gram in doc/rand.rst

3c29e72

fix: ruff check error

5e2dfe0

purva-thakre reviewed Dec 26, 2023

View reviewed changes

fix: added doc string to random_circulant_gram func

4c8b731

vprusso reviewed Dec 28, 2023

View reviewed changes

purva-thakre reviewed Dec 29, 2023

View reviewed changes

ankit-pn added 3 commits December 29, 2023 10:22

fix: format and add citation

35615f1

fix: modify docs and _init_.py according to alphabetical order

64938b6

fix: fix test_random_circulant_gram.

255a1bc

purva-thakre reviewed Dec 29, 2023

View reviewed changes

toqito/rand/__init__.py Show resolved Hide resolved

purva-thakre reviewed Dec 29, 2023

View reviewed changes

toqito/rand/random_circulant_gram.py Outdated Show resolved Hide resolved

purva-thakre reviewed Dec 29, 2023

View reviewed changes

toqito/rand/random_circulant_gram.py Outdated Show resolved Hide resolved

ankit-pn and others added 2 commits December 29, 2023 19:51

Update toqito/rand/random_circulant_gram.py

ace0972

Co-authored-by: Purva Thakre <66048318+purva-thakre@users.noreply.github.com>

Update toqito/rand/random_circulant_gram.py

8df2165

Co-authored-by: Purva Thakre <66048318+purva-thakre@users.noreply.github.com>

purva-thakre reviewed Dec 29, 2023

View reviewed changes

toqito/rand/random_circulant_gram.py Outdated Show resolved Hide resolved

ankit-pn and others added 5 commits December 29, 2023 19:59

Update toqito/rand/random_circulant_gram.py

bef082e

Co-authored-by: Purva Thakre <66048318+purva-thakre@users.noreply.github.com>

fix: docs update

c0969fb

fix: doc

464adfa

fix: doc

a134c80

fix: doc

3f061cd

purva-thakre reviewed Dec 29, 2023

View reviewed changes

toqito/rand/random_circulant_gram.py Show resolved Hide resolved

ankit-pn and others added 2 commits December 29, 2023 20:25

Update toqito/rand/random_circulant_gram.py

2870c03

Co-authored-by: Purva Thakre <66048318+purva-thakre@users.noreply.github.com>

fix: docs

6cabc01

purva-thakre reviewed Jan 10, 2024

View reviewed changes

toqito/rand/random_circulant_gram.py Outdated Show resolved Hide resolved

Update toqito/rand/random_circulant_gram.py

181d567

Co-authored-by: Purva Thakre <66048318+purva-thakre@users.noreply.github.com>

vprusso added 2 commits January 10, 2024 14:38

Update random_circulant_gram.py

a2d43dc

Fixing linter.

Update random_circulant_gram.py

2ac70b4

Removing whitespace.

vprusso merged commit 4f75605 into vprusso:master Jan 10, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add rand_circulant_gram func #389

feat: add rand_circulant_gram func #389

ankit-pn commented Dec 22, 2023 •

edited by purva-thakre

Loading

codecov bot commented Dec 22, 2023 •

edited

Loading

purva-thakre Dec 26, 2023

ankit-pn Dec 27, 2023

purva-thakre Dec 27, 2023

vprusso Dec 28, 2023

ankit-pn commented Dec 27, 2023

vprusso Dec 28, 2023

vprusso Dec 28, 2023

purva-thakre Dec 28, 2023

ankit-pn Dec 29, 2023

vprusso Dec 28, 2023

ankit-pn Dec 29, 2023

purva-thakre Dec 29, 2023

ankit-pn Dec 29, 2023

purva-thakre Dec 29, 2023 •

edited

Loading

ankit-pn commented Dec 29, 2023

vprusso commented Jan 7, 2024

purva-thakre commented Jan 7, 2024

vprusso commented Jan 10, 2024

feat: add rand_circulant_gram func #389

feat: add rand_circulant_gram func #389

Conversation

ankit-pn commented Dec 22, 2023 • edited by purva-thakre Loading

Description

Status

codecov bot commented Dec 22, 2023 • edited Loading

Codecov Report

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

ankit-pn commented Dec 27, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purva-thakre Dec 29, 2023 • edited Loading

Choose a reason for hiding this comment

ankit-pn commented Dec 29, 2023

vprusso commented Jan 7, 2024

purva-thakre commented Jan 7, 2024

vprusso commented Jan 10, 2024

ankit-pn commented Dec 22, 2023 •

edited by purva-thakre

Loading

codecov bot commented Dec 22, 2023 •

edited

Loading

purva-thakre Dec 29, 2023 •

edited

Loading