Fix NormalCG shape bug with rectangular matrices#186
Closed
jpbrodrick89 wants to merge 1 commit intopatrick-kidger:mainfrom
Closed
Fix NormalCG shape bug with rectangular matrices#186jpbrodrick89 wants to merge 1 commit intopatrick-kidger:mainfrom
jpbrodrick89 wants to merge 1 commit intopatrick-kidger:mainfrom
Conversation
The bug was introduced in commit 6893255 which moved the preconditioner_and_y0() call before the vector transformation for NormalCG mode. This caused y0 to be initialized in the wrong space. For rectangular matrices (m×n where m>n), NormalCG transforms the vector from output space (m,) to input space (n,) via A^T. The y0 initial guess must be in this transformed space, but was being initialized before the transformation. This fix: - Moves preconditioner_and_y0() to after vector transformation - Preserves the preconditioner transformation logic added in 6893255 - Ensures y0 has the correct shape for both square and rectangular matrices - Adds NormalCG to rectangular matrix tests to prevent regression Fixes issue with NormalCG failing on overdetermined systems with: TypeError: dot_general requires contracting dimensions to have the same shape, got (3,) and (6,). All 3587 tests pass.
Owner
|
Thanks for the fix! I am actually (finally...) about to do a new release over in #189, which I think will supersede this. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug was introduced in commit 6893255 which moved the preconditioner_and_y0() call before the vector transformation for NormalCG mode. This caused y0 to be initialized in the wrong space. This was not caught previously because NormalCG was omitted from rectangular matrix tests! I realise this is fixed on
devwith super clean and awesomelx.Normalimplementation! ❤️ But this provides a temporary fix onmainuntildevis merged in, these changes can be ignored at that point.For rectangular matrices (m×n where m>n), NormalCG transforms the vector from output space (m,) to input space (n,) via A^T. The y0 initial guess must be in this transformed space, but was being initialized before the transformation.
This fix:
Fixes issue with NormalCG failing on overdetermined systems with:
TypeError: dot_general requires contracting dimensions to have the same shape, got (3,) and (6,).