extend Cholesky decomposition method for finite fields #39200

jacksonwalters · 2024-12-24T21:23:41Z

Given a Hermitian matrix $U$ over $GF(q^2)$, returns a matrix $A$ such that $U = AA^\ast$, where $^\ast$ denotes conjugate-transpose.

Note that the Cholesky decomposition is meant for characteristic zero, and also only returns a lower/upper triangular matrix.

Corresponding: Issue #39202
Discussion: Issue #38456
Required by: PR #38455

📝 Checklist

The title is concise and informative.
The description explains in detail what this PR is about.
I have linked a relevant issue or discussion.
I have created tests covering the changes.
I have updated the documentation and checked the documentation preview.

⌛ Dependencies

github-actions · 2024-12-26T19:36:00Z

Documentation preview for this PR (built with commit ae8eab0; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

src/sage/matrix/matrix2.pyx

tscrim · 2024-12-30T00:38:25Z

I really dislike these apply-suggestion-commits with a completely useless commit message. Could you please squash and rebase? It makes it easier to see which changes have been done and looking through the logs much later.

jacksonwalters · 2024-12-30T00:56:13Z

Sure. I agree, the commit messages are pretty useless. I've squashed the commits here. Which branch should I rebase on? I rebased PR#38455 on this one (PR#39200) already.

tscrim · 2024-12-30T01:03:43Z

Thanks. Sorry, I misspoke slightly, I meant force-push.You will need to rebase #38455 again though…

jacksonwalters · 2024-12-30T01:22:05Z

@tscrim Okay, I have rebased #38455 on this again and attempted to resolve merge conflicts.

jacksonwalters · 2024-12-30T02:32:54Z

I don't think this handles the 1x1 case, so I'll add a fix.

jacksonwalters · 2025-01-02T20:00:03Z

@tscrim How do I squash these commits given there's a merge commit? We may've done this before, but I don't see how.

tscrim · 2025-01-04T06:47:14Z

I would just rebase the entire branch over the latest develop branch and squash commits as reasonable.

tscrim

Just a few more details, modulo making row 0-based.

src/sage/matrix/matrix2.pyx

tscrim · 2025-01-04T06:49:19Z

src/sage/matrix/matrix2.pyx

+            row += 1
+
+            # Look for a non-zero element on the main diagonal, starting from `row`
+            i = row - 1  # Adjust for zero-based indexing in Sage


I don't think this is possible as we mutate i compared to row.

jacksonwalters · 2025-01-15T16:02:00Z

@tscrim I've refactored it so that row is initialized to -1, so that many row - 1 statements are just row. I agree, I don't think we can decouple it from i in any way.

tscrim

As I look into the Cholesky decomposition and its doc, I am thinking we should actually call this method cholesky_extended() for "extended Cholesky decomposition". Or possibly accessing this via cholesky(extended=True) (with the default being False), in which case this method would be called something like _cholesky_extended_ff. I am actually leaning towards the latter right now since it is the most natural. Sorry for a somewhat big change so far along.

This paper is also somewhat relevant: journal version; [arXiv version](https://arxiv.org/abs/2202.04012v1.

src/sage/matrix/matrix2.pyx

jacksonwalters · 2025-01-17T19:25:38Z

I'm happy to have this accessed via cholesky(extended=True). That's really how we came upon it, trying to do the Cholesky decomposition over finite fields but realizing it didn't work for some matrices. It also makes it more likely other people will see it and use it. I'll attempt to move it.

jacksonwalters · 2025-01-17T19:50:20Z

Okay, I have moved the code into cholesky(). It may require some changes. I believe Dima and I did look at that paper when trying to figure this out, but it didn't have exactly this method. Dima found it in GAP.

src/sage/matrix/matrix2.pyx

dimpase · 2025-01-30T17:39:38Z

please rebase over the latest beta (or merge it in)

dimpase · 2025-01-30T17:41:37Z

otherwise, it's ready to go. CI failures (you can inspect what tests fail) are not relevant

jacksonwalters · 2025-01-30T18:18:13Z

What is the process for rebasing over the latest beta? Is it: git fetch origin git checkout develop git pull origin develop git checkout unitary_dft_symmetric_group git rebase develop Thanks, Jackson

…

On Thu, Jan 30, 2025 at 12:41 PM Dima Pasechnik ***@***.***> wrote: otherwise, it's ready to go. CI failures (you can inspect what tests fail) are not relevant — Reply to this email directly, view it on GitHub <#39200 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIX5PYFQ5HO7Z4IISO2C5D2NJP6NAVCNFSM6AAAAABUFGB432VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMMRVGE3DEMJRGI> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

jacksonwalters · 2025-01-30T18:30:53Z

***@***.*** sage % git fetch origin ***@***.*** sage % git checkout develop Already on 'develop' Your branch is up to date with 'origin/develop'. ***@***.*** sage % git pull origin develop From https://github.com/jacksonwalters/sage * branch develop -> FETCH_HEAD Already up to date. ***@***.*** sage % git checkout unitary_dft_symmetric_group Updating files: 100% (5535/5535), done. Switched to branch 'unitary_dft_symmetric_group' Your branch is up to date with 'origin/unitary_dft_symmetric_group'. ***@***.*** sage % git rebase develop Auto-merging src/sage/groups/artin.py Auto-merging src/sage/groups/cubic_braid.py Auto-merging src/sage/groups/finitely_presented.py Auto-merging src/sage/groups/free_group.py CONFLICT (content): Merge conflict in src/sage/groups/free_group.py Auto-merging src/sage/groups/raag.py Auto-merging src/sage/libs/gap/element.pyx error: could not apply e9222c4... Added gap groups for free and f.p groups, added __reduce__ methods hint: Resolve all conflicts manually, mark them as resolved with hint: "git add/rm <conflicted_files>", then run "git rebase --continue". hint: You can instead skip this commit: run "git rebase --skip". hint: To abort and get back to the state before "git rebase", run "git rebase --abort". hint: Disable this message with "git config advice.mergeConflict false" Could not apply e9222c4... Added gap groups for free and f.p groups, added __reduce__ methods for these groups and also for artin, cubicbraid and raag On Thu, Jan 30, 2025 at 1:17 PM Jackson Walters ***@***.***> wrote:

…

What is the process for rebasing over the latest beta? Is it: git fetch origin git checkout develop git pull origin develop git checkout unitary_dft_symmetric_group git rebase develop Thanks, Jackson On Thu, Jan 30, 2025 at 12:41 PM Dima Pasechnik ***@***.***> wrote: > otherwise, it's ready to go. CI failures (you can inspect what tests > fail) are not relevant > > — > Reply to this email directly, view it on GitHub > <#39200 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAIX5PYFQ5HO7Z4IISO2C5D2NJP6NAVCNFSM6AAAAABUFGB432VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMMRVGE3DEMJRGI> > . > You are receiving this because you modified the open/close state.Message > ID: ***@***.***> >

dimpase · 2025-01-30T21:16:37Z

to rebase a local branch foo42 over the current develop, do

git checkout foo42
git fetch origin develop
git rebase origin/develop 
# it might start asking you questions

tscrim · 2025-01-31T05:14:06Z

I do something slightly different:

git checkout foo42
git fetch upstream develop:develop
git rebase develop
# deal with merge conflicts; "git rebase --continue" once resolved
git push -f origin foo42

dimpase · 2025-01-31T06:00:59Z

you probably meant git rebase develop

tscrim · 2025-01-31T06:02:16Z

you probably meant git rebase develop

Yes, I did. Thanks. Fixed.

jacksonwalters · 2025-02-26T22:59:17Z

@tscrim @dimpase Okay, I've rebased this over develop. Seems like there are still some issues with the CI tests, not sure how to resolve those.

tscrim · 2025-02-27T00:07:42Z

They don't look related and some of them are known to be flaky. I wouldn't worry about the failures.

tscrim · 2025-02-27T00:40:00Z

@jacksonwalters What about the last round of my suggested changes? Any questions/comments/disagreements?

jacksonwalters · 2025-02-27T22:26:36Z

@jacksonwalters What about the last round of my suggested changes? Any questions/comments/disagreements?

My apologies, I think I saw those right as I was getting caught up with another project. Those have been addressed now. The docs for _cholesky_extended_ff() could probably be elaborated a bit, and just want to make sure having the input matrix be nonsingular is a necessary condition.

jacksonwalters · 2025-02-27T22:28:36Z

They don't look related and some of them are known to be flaky. I wouldn't worry about the failures.

Yes, they seem to be flaky. I guess my concern is that this PR will not be resolved until those start passing. That could be indefinite it seems?

tscrim · 2025-02-27T23:04:28Z

They don't look related and some of them are known to be flaky. I wouldn't worry about the failures.

Yes, they seem to be flaky. I guess my concern is that this PR will not be resolved until those start passing. That could be indefinite it seems?

No, it will get merged even if the automated tests work. As long as the actual tests pass (when run by humans on more standard machines), then it will get merged.

tscrim

Thank you. Just a few more little things. (It would be nice to squash some/all of the commits though since there are a lot of completely unhelpful commit messages...)

src/sage/matrix/matrix2.pyx

tscrim · 2025-02-28T00:52:26Z

src/sage/matrix/matrix2.pyx

+        if self.is_singular():
+            raise ValueError("matrix must be nonsingular")


Suggested change

if self.is_singular():

raise ValueError("matrix must be nonsingular")

I think this is somewhat expensive check that is unnecessary since it fails during the process. A division by 0 right? That would be the place to check (or perhaps catch the error).

Also, please add a doctest for the singular case (just in the helper method).

That's the thing, it still produces an output matrix, it just doesn't satisfy the B*B.H == U. For example, U = matrix(GF(3**2),[[1,4,7],[4,1,4],[7,4,1]]) yields B = matrix(GF(3**2),[[1 0 0],[1 1 0],[1 0 1]]). So we can only catch the error at the end, when it fails.

Okay, I've added a (failing) singular example for now. I don't think there's a division by zero. It must be towards the end of the method where the rank is computed, and if it's not full rank it causes the issue.

The above fail case is rank 1. The following fail case is rank 2: U = matrix(F,[[0,4,7],[4,1,4],[7,4,1]]). Note that at the end of the row reduction, we are already computing r, which is the rank of the input matrix. I propose we just check that the matrix is full rank, since it appears that it needs to be. Does it?

No, it doesn't. The example U = matrix(GF(3**2),[[0,0,0],[0,1,0],[0,0,1]]) should just return the same matrix. It returns B = matrix(GF(3**2),[[0 0 1],[1 0 0],[0 1 0]]), which does not satisfy B*B.H == U.

It seems like in the initial While loop, it will note the first diagonal entry is zero, then try to do row/col swaps to ensure it's nonzero. I think for rank deficient matrices there may just be no way to do row/col swaps to get all diagonal entries nonzero. So we're left with a "bad" matrix, when really we shouldn't've done any swaps at all.

jacksonwalters · 2025-03-03T21:52:09Z

@tscrim Squashed above commits. There are some singular cases it should handle, though. Namely, U = matrix(GF(3**2),[[0,0,0],[0,1,0],[0,0,1]]) is a rank two matrix, and the output should just be matrix(GF(3**2),[[0,0,0],[0,1,0],[0,0,1]]). Even if we allow the computation to proceed, we get the wrong result: B = matrix(GF(3**2),[[0 0 1],[1 0 0],[0 1 0]]).

add hermitian_decomposition method given a Hermitian matrix U, returns a matrix A such that U = AA* use translated GAP source code for BaseChangeCanonical raise ValueError for non full rank matrices this method does not work for singular matrices. since we are already computing the rank in the row reduction, we can just check at the end whether the rank is full, and if not exit with a ValueError. this may be overcautious - it's not clear that this won't work for some singular matrices. there are cases where it certainly doesn't work, and we include one in the doctest. add example for singular case this example is currently failing and needs to be caught remove unnecessary import, singularity check if the matrix `U` is singular, the process will still result in a matrix but it will fail to have the expected property, namely `B*B.H == U`. Update src/sage/matrix/matrix2.pyx avoid import, directly call `sqrt` method, don't extend to keep result in `ZZ` fix example this was using the output from the `forms` package, when it should be from the GAP source translation. change examples there is a failing example due to the fact that the matrix is singular with q=3, and U = matrix(F,[[1,4,7],[4,1,4],[7,4,1]]). move _cholesky_extended_ff to hidden method move _cholesky_extended_ff to a separate hidden method, and just call it from inside cholesky if extended=true. add doctests and docs for the method separately. sparse for now, should be expanded upon. Update src/sage/matrix/matrix2.pyx add output for GF(3**2) case this case fails to have a lower triangular decomposition which has been checked by brute force. Update src/sage/matrix/matrix2.pyx move two examples over finite fields from tests the two examples computing the extended Cholesky decomposition over square order finite fields are in TESTS, and should be moved to EXAMPLES. Update src/sage/matrix/matrix2.pyx this is better and clarifies that the result of the Cholesky decomposition is lower triangular (and not possibly upper triangular). Update src/sage/matrix/matrix2.pyx add single whitespace before `extended` parameter Update src/sage/matrix/matrix2.pyx looks good. makes sense to point out the Cholesky decomposition might not exist, given that was our initial difficulty. 3 trailing whitespace move hermitian_decomposition into cholesky this method for decomposition a Hermitian matrix U = AA* is similar in spirit to the Cholesky decomposition, but extends it to work over finite fields of square order. add two examples add two examples of a Hermitian decomposition, one of which is not upper/lower triangular (so would be impossible with Cholesky decomposition) Update src/sage/matrix/matrix2.pyx yes, just forgot to update this initialize row to -1 Update src/sage/matrix/matrix2.pyx add word "package" to GAP ``forms`` Update src/sage/matrix/matrix2.pyx change \ast to * for consistency Trigger CI tests use self.__class__ within matrix.pyx file Trigger CI tests remove blank line Update src/sage/matrix/matrix2.pyx Update src/sage/matrix/matrix2.pyx Update src/sage/matrix/matrix2.pyx Update src/sage/matrix/matrix2.pyx Update src/sage/matrix/matrix2.pyx use Matrix handle 1x1 case format failing test case properly format failing test case properly by removing variables that hold results of computation. just call the function that raises the exception Co-Authored-By: Travis Scrimshaw <[email protected]>

forgot the colon

tscrim

Thank you for everything. Two last little details to try and make it play better with the other methods.

src/sage/matrix/matrix2.pyx

cache the rank of the matrix since are computing it anyways Co-authored-by: Travis Scrimshaw <[email protected]>

fetch the cached rank. if it's full or not yet computed, continue, otherwise throw a ValueError that the matrix is not full rank. Co-authored-by: Travis Scrimshaw <[email protected]>

jacksonwalters · 2025-03-04T02:02:12Z

Thank you for everything. Two last little details to try and make it play better with the other methods.

Nice, I've committed those changes. At this point I'm happy to just have it work for full rank matrices (since that is all we need for 38455.

However, I'm wondering if setting D = identity_matrix(F,n) is part of the issue. For a rank deficient matrix of rank r < n, shouldn't we use a matrix with r one's on the diagonal instead?

tscrim

I'm not sure about the mathematics off-hand. I'd say let's go with this for now, and we can come back to it later if there is a good use-case for it.

If the tests pass (morally), then I will set this to a positive review.

jacksonwalters · 2025-03-04T02:20:32Z

I'm not sure about the mathematics off-hand. I'd say let's go with this for now, and we can come back to it later if there is a good use-case for it.

If the tests pass (morally), then I will set this to a positive review.

Sounds good. I agree that it would be better to handle it in a separate PR if needed. I tried initializing D to have r one's on the diagonal, and it was still off, so it's something subtle with the swaps.

github-actions bot added the s: needs review label Dec 24, 2024

jacksonwalters mentioned this pull request Dec 24, 2024

unitary DFT for symmetric group algebra #38455

Open

5 tasks

jacksonwalters closed this by deleting the head repository Dec 26, 2024

jacksonwalters reopened this Dec 27, 2024

github-actions bot removed the s: needs review label Dec 27, 2024

jacksonwalters force-pushed the hermitian_decomposition branch from caed579 to f2f35b7 Compare December 27, 2024 14:38

github-actions bot added the s: needs review label Dec 27, 2024

tscrim requested changes Dec 29, 2024

View reviewed changes

jacksonwalters force-pushed the hermitian_decomposition branch from 6300bae to a8cd89e Compare December 30, 2024 00:54

tscrim requested changes Jan 4, 2025

View reviewed changes

tscrim reviewed Jan 17, 2025

View reviewed changes

jacksonwalters changed the title ~~add hermitian_decomposition method~~ extend Cholesky decomposition method for finite fields of square order Jan 18, 2025

jacksonwalters changed the title ~~extend Cholesky decomposition method for finite fields of square order~~ extend Cholesky decomposition method for finite fields Jan 18, 2025

tscrim requested changes Jan 20, 2025

View reviewed changes

jacksonwalters force-pushed the hermitian_decomposition branch from 704ae31 to c59ff69 Compare February 26, 2025 21:45

tscrim requested changes Feb 28, 2025

View reviewed changes

jacksonwalters force-pushed the hermitian_decomposition branch from f24dfd7 to cefde12 Compare March 3, 2025 21:44

jacksonwalters force-pushed the hermitian_decomposition branch from 5b519ea to 7c27ee4 Compare March 3, 2025 22:29

jacksonwalters added 2 commits March 3, 2025 17:53

fail cases should be tests, not examples

2367393

add colon after Traceback

16db31c

forgot the colon

tscrim requested changes Mar 4, 2025

View reviewed changes

src/sage/matrix/matrix2.pyx Outdated Show resolved Hide resolved

src/sage/matrix/matrix2.pyx Outdated Show resolved Hide resolved

jacksonwalters and others added 2 commits March 3, 2025 20:55

Update src/sage/matrix/matrix2.pyx

68ab75c

cache the rank of the matrix since are computing it anyways Co-authored-by: Travis Scrimshaw <[email protected]>

Update src/sage/matrix/matrix2.pyx

ae8eab0

fetch the cached rank. if it's full or not yet computed, continue, otherwise throw a ValueError that the matrix is not full rank. Co-authored-by: Travis Scrimshaw <[email protected]>

tscrim approved these changes Mar 4, 2025

View reviewed changes

		if self.is_singular():
		raise ValueError("matrix must be nonsingular")

extend Cholesky decomposition method for finite fields #39200

Are you sure you want to change the base?

extend Cholesky decomposition method for finite fields #39200

Conversation

jacksonwalters commented Dec 24, 2024 • edited Loading

📝 Checklist

⌛ Dependencies

github-actions bot commented Dec 26, 2024 • edited Loading

tscrim commented Dec 30, 2024

jacksonwalters commented Dec 30, 2024

tscrim commented Dec 30, 2024

jacksonwalters commented Dec 30, 2024

jacksonwalters commented Dec 30, 2024

jacksonwalters commented Jan 2, 2025

tscrim commented Jan 4, 2025

tscrim left a comment

Choose a reason for hiding this comment

tscrim Jan 4, 2025

Choose a reason for hiding this comment

jacksonwalters commented Jan 15, 2025

tscrim left a comment

Choose a reason for hiding this comment

jacksonwalters commented Jan 17, 2025

jacksonwalters commented Jan 17, 2025

dimpase commented Jan 30, 2025

dimpase commented Jan 30, 2025

jacksonwalters commented Jan 30, 2025 via email

jacksonwalters commented Jan 30, 2025 via email

dimpase commented Jan 30, 2025

tscrim commented Jan 31, 2025 • edited Loading

dimpase commented Jan 31, 2025

tscrim commented Jan 31, 2025

jacksonwalters commented Feb 26, 2025 • edited Loading

tscrim commented Feb 27, 2025

tscrim commented Feb 27, 2025

jacksonwalters commented Feb 27, 2025

jacksonwalters commented Feb 27, 2025

tscrim commented Feb 27, 2025

tscrim left a comment

Choose a reason for hiding this comment

tscrim Feb 28, 2025

Choose a reason for hiding this comment

jacksonwalters Mar 3, 2025

Choose a reason for hiding this comment

jacksonwalters Mar 3, 2025

Choose a reason for hiding this comment

jacksonwalters Mar 3, 2025

Choose a reason for hiding this comment

jacksonwalters Mar 3, 2025

Choose a reason for hiding this comment

jacksonwalters Mar 3, 2025

Choose a reason for hiding this comment

jacksonwalters commented Mar 3, 2025

tscrim left a comment

Choose a reason for hiding this comment

jacksonwalters commented Mar 4, 2025

tscrim left a comment

Choose a reason for hiding this comment

jacksonwalters commented Mar 4, 2025 • edited Loading

jacksonwalters commented Dec 24, 2024 •

edited

Loading

github-actions bot commented Dec 26, 2024 •

edited

Loading

tscrim commented Jan 31, 2025 •

edited

Loading

jacksonwalters commented Feb 26, 2025 •

edited

Loading

jacksonwalters commented Mar 4, 2025 •

edited

Loading