Changes in Gram-Schmidt functions #239

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.

Sign up for GitHub

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

Jump to bottom

Merged

cval26 merged 7 commits into master from orthogonalize-last

Dec 15, 2023

Collaborator

cval26 commented Sep 23, 2023 •

edited

Loading

This PR includes two main changes.

Fixes issues in the current orthogonalize method of the Matrix class.
Adds an incremental Gram-Schmidt function orthogonalize_last, where only the last column of the input matrix is orthonormalized, assuming the previous columns are already orthonormal.

The above changes are motivated by applications in the Laghos hydrodynamics code.

cval26 added bug enhancement WIP labels

cval26 self-assigned this

cval26 requested review from chldkdtn and dylan-copeland

September 23, 2023 20:48

cval26 commented

View reviewed changes

lib/linalg/Matrix.cpp Show resolved Hide resolved

cval26 commented

View reviewed changes

lib/linalg/Matrix.h Outdated Show resolved Hide resolved

cval26 force-pushed the orthogonalize-last branch from d97827b to c421495 Compare

September 27, 2023 15:35

cval26 added RFR and removed WIP labels

cval26 marked this pull request as ready for review

September 27, 2023 15:43

Collaborator

dylan-copeland commented Sep 27, 2023

The style script needs to be run.

Collaborator Author

cval26 commented Sep 27, 2023

@dylan-copeland
I just did. Sorry about that; didn't run it right the first time.

Collaborator Author

cval26 commented Sep 27, 2023 •

edited

Loading

The tests I added in the Matrix test suite fail in the CI build, even though they run successfully in my local build on LC. I'll have to check why and get back to you.

@dylan-copeland
Looking at the CI output, it looks like the tests are run once successfully, then run once again and they fail. Is this somehow related to the warning at the beginning of the test output file saying that two active threads have been detected? I'm not sure if the problem is in the unit tests themselves or in the way they were run.

EDIT: The issue has been resolved by the changes made after review number 1.

dylan-copeland reviewed

View reviewed changes

lib/linalg/Matrix.cpp Outdated Show resolved Hide resolved

dylan-copeland reviewed

View reviewed changes

lib/linalg/Matrix.cpp Outdated Show resolved Hide resolved

dylan-copeland reviewed

View reviewed changes

lib/linalg/Matrix.cpp Outdated Show resolved Hide resolved

dylan-copeland reviewed

View reviewed changes

lib/linalg/Matrix.cpp Outdated Show resolved Hide resolved

cval26 requested a review from dylan-copeland

September 28, 2023 16:29

Collaborator Author

cval26 commented Sep 28, 2023

@dylan-copeland Thank you for the review comments; the testing issue has been resolved.

@dylan-copeland @chldkdtn Please review the code once more so that we can merge it.

cval26 mentioned this pull request

Matrix::orthogonalize() works only for distributed case. #232

Open

dylan-copeland reviewed

View reviewed changes

lib/linalg/Matrix.cpp Show resolved Hide resolved

dylan-copeland reviewed

View reviewed changes

lib/linalg/Matrix.cpp Outdated Show resolved Hide resolved

dylan-copeland reviewed

View reviewed changes

lib/linalg/Matrix.cpp Show resolved Hide resolved

dylan-copeland reviewed

View reviewed changes

lib/linalg/Matrix.h Outdated Show resolved Hide resolved

dylan-copeland reviewed

View reviewed changes

lib/linalg/Matrix.cpp Outdated Show resolved Hide resolved

chldkdtn requested review from ckendrick, dreamer2368 and ebchin

October 17, 2023 06:24

chldkdtn requested review from andersonw1 and siuwuncheung

October 17, 2023 06:24

cval26 requested review from chldkdtn, dylan-copeland and siuwuncheung and removed request for chldkdtn and siuwuncheung

November 1, 2023 15:36

dylan-copeland reviewed

View reviewed changes

lib/linalg/Matrix.h Outdated Show resolved Hide resolved

dylan-copeland reviewed

View reviewed changes

lib/linalg/Matrix.h Outdated Show resolved Hide resolved

Collaborator

dylan-copeland commented Nov 2, 2023

git pull origin master will resolve the conflicts and should make the regression tests pass (currently they are using wrong command line flags).

cval26 added 7 commits

November 3, 2023 08:43


          fix issues in Matrix::orthogonalize

2ff3ee0


          add Matrix::orthogonalize_last method

698b157


          run astyle

d1c65a2


          add another test for Matrix::orthogonalize_last

9cbc970


          rerun astyle properly

6f1a113


          review n.1 changes

f4893d9


          review n.2

419ba38

cval26 force-pushed the orthogonalize-last branch from 3083bea to 419ba38 Compare

November 3, 2023 15:49

cval26 requested a review from dylan-copeland

November 3, 2023 15:50

siuwuncheung approved these changes

View reviewed changes

dylan-copeland approved these changes

View reviewed changes

cval26 merged commit a082bd3 into master

cval26 deleted the orthogonalize-last branch

December 18, 2023 18:35

andersonw1 pushed a commit that referenced this pull request


          Changes in Gram-Schmidt functions (#239)

583881c

* fix issues in Matrix::orthogonalize

* add Matrix::orthogonalize_last method

* run astyle

* add another test for Matrix::orthogonalize_last

* rerun astyle properly

* review n.1 changes

* review n.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

dylan-copeland dylan-copeland approved these changes

siuwuncheung siuwuncheung approved these changes

ckendrick Awaiting requested review from ckendrick

ebchin Awaiting requested review from ebchin

dreamer2368 Awaiting requested review from dreamer2368

andersonw1 Awaiting requested review from andersonw1

chldkdtn Awaiting requested review from chldkdtn

Labels

bug enhancement RFR