Skip to content

Gram-Schmidt with R#577

Merged
pmli merged 3 commits into
masterfrom
gram_schmidt-R
Feb 2, 2019
Merged

Gram-Schmidt with R#577
pmli merged 3 commits into
masterfrom
gram_schmidt-R

Conversation

@pmli

@pmli pmli commented Jan 31, 2019

Copy link
Copy Markdown
Member

Closes #491.

  • adds an option to compute the R matrix
  • some improvements (no more flake8 issues with old_norm)
  • adds unit tests

@pmli pmli added pr:new-feature Introduces a new feature linear algebra labels Jan 31, 2019
@pmli pmli added this to the 2019.2 milestone Jan 31, 2019
@pmli pmli requested a review from sdrave January 31, 2019 19:07
@codecov

codecov Bot commented Feb 1, 2019

Copy link
Copy Markdown

Codecov Report

Merging #577 into master will decrease coverage by <.01%.
The diff coverage is 94.44%.

Impacted Files Coverage Δ
src/pymor/algorithms/gram_schmidt.py 98% <94.44%> (+0.12%) ⬆️
src/pymor/reductors/residual.py 69.72% <0%> (-0.62%) ⬇️

@codecov

codecov Bot commented Feb 1, 2019

Copy link
Copy Markdown

Codecov Report

Merging #577 into master will decrease coverage by 0.36%.
The diff coverage is 93.75%.

Impacted Files Coverage Δ
src/pymor/algorithms/gram_schmidt.py 97.89% <93.75%> (+0.02%) ⬆️
src/pymor/reductors/residual.py 69.72% <0%> (-0.62%) ⬇️
src/pymor/models/iosys.py 61.48% <0%> (ø)
src/pymor/models/mpi.py 26.66% <0%> (ø)
src/pymor/models/basic.py 85.93% <0%> (ø)
src/pymor/models/interfaces.py 83.33% <0%> (ø)
src/pymor/reductors/bt.py 66.08% <0%> (+1.5%) ⬆️
src/pymor/discretizers/fv.py 88.38% <0%> (+4.92%) ⬆️

@sdrave sdrave left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Just one remark: Since computing R is mostly for free, I would simplify the code by always computing R and renaming the parameter from compute_R to return_R

@pmli

pmli commented Feb 1, 2019

Copy link
Copy Markdown
Member Author

Could allocating additional memory cause issues when return_R == False? Or can len(A) always be assumed to be "small"? I was also wondering if R should be constructed row by row, instead of creating a square matrix and deleting rows at the end.

@sdrave

sdrave commented Feb 1, 2019

Copy link
Copy Markdown
Member

I would say that if len(A) is so large, that storing len(A)**2 doubles is problematic, then a VectorArray is the wrong data structure.

@pmli

pmli commented Feb 1, 2019

Copy link
Copy Markdown
Member Author

Ok, good. I made the changes.

@pmli pmli merged commit dbd8db4 into master Feb 2, 2019
@pmli pmli deleted the gram_schmidt-R branch February 2, 2019 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr:new-feature Introduces a new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants