Skip to content
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

extend Cholesky decomposition method for finite fields #39200

Open
wants to merge 26 commits into
base: develop
Choose a base branch
from

Conversation

jacksonwalters
Copy link
Contributor

@jacksonwalters jacksonwalters commented Dec 24, 2024

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.

📝 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

Copy link

github-actions bot commented Dec 26, 2024

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

@jacksonwalters jacksonwalters closed this by deleting the head repository Dec 26, 2024
@jacksonwalters jacksonwalters force-pushed the hermitian_decomposition branch from caed579 to f2f35b7 Compare December 27, 2024 14:38
src/sage/matrix/matrix2.pyx Outdated Show resolved Hide resolved
src/sage/matrix/matrix2.pyx Outdated Show resolved Hide resolved
src/sage/matrix/matrix2.pyx Outdated Show resolved Hide resolved
src/sage/matrix/matrix2.pyx Outdated Show resolved Hide resolved
src/sage/matrix/matrix2.pyx Outdated Show resolved Hide resolved
src/sage/matrix/matrix2.pyx Outdated Show resolved Hide resolved
src/sage/matrix/matrix2.pyx Outdated Show resolved Hide resolved
src/sage/matrix/matrix2.pyx Outdated Show resolved Hide resolved
src/sage/matrix/matrix2.pyx Outdated Show resolved Hide resolved
src/sage/matrix/matrix2.pyx Outdated Show resolved Hide resolved
@tscrim
Copy link
Collaborator

tscrim commented Dec 30, 2024

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.

add hermitian_decomposition method

given a Hermitian matrix U, returns a matrix A such that U = AA*

use translated GAP source code for BaseChangeCanonical

Co-Authored-By: Travis Scrimshaw <clfrngrown@aol.com>
@jacksonwalters jacksonwalters force-pushed the hermitian_decomposition branch from 6300bae to a8cd89e Compare December 30, 2024 00:54
@jacksonwalters
Copy link
Contributor Author

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
Copy link
Collaborator

tscrim commented Dec 30, 2024

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

@jacksonwalters
Copy link
Contributor Author

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

@jacksonwalters
Copy link
Contributor Author

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

jacksonwalters and others added 10 commits December 29, 2024 19:49
Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
@jacksonwalters
Copy link
Contributor Author

@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
Copy link
Collaborator

tscrim commented Jan 4, 2025

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

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/sage/matrix/matrix2.pyx Outdated Show resolved Hide resolved
src/sage/matrix/matrix2.pyx Outdated Show resolved Hide resolved
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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

jacksonwalters and others added 3 commits January 15, 2025 10:27
change \ast to * for consistency

Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
add word "package" to GAP ``forms``

Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
@jacksonwalters
Copy link
Contributor Author

@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.

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
src/sage/matrix/matrix2.pyx Outdated Show resolved Hide resolved
src/sage/matrix/matrix2.pyx Outdated Show resolved Hide resolved
src/sage/matrix/matrix2.pyx Outdated Show resolved Hide resolved
src/sage/matrix/matrix2.pyx Outdated Show resolved Hide resolved
jacksonwalters and others added 5 commits January 17, 2025 13:58
yes, just forgot to update this

Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
add two examples of a Hermitian decomposition, one of which is not upper/lower triangular (so would be impossible with Cholesky decomposition)
replace always True with explicit conditional

Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
remove this break conditional in favor of conditional for while loop

Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
add a SEEALSO since this is an extended form of the Cholesky decomposition

Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
@jacksonwalters
Copy link
Contributor Author

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.

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.
@jacksonwalters
Copy link
Contributor Author

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.

@jacksonwalters jacksonwalters changed the title add hermitian_decomposition method extend Cholesky decomposition method for finite fields of square order Jan 18, 2025
@jacksonwalters jacksonwalters changed the title extend Cholesky decomposition method for finite fields of square order extend Cholesky decomposition method for finite fields Jan 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants