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

add Gram-Schmidt process #11 #15

Merged
merged 21 commits into from
Jul 2, 2020
Merged

add Gram-Schmidt process #11 #15

merged 21 commits into from
Jul 2, 2020

Conversation

felipeZ
Copy link
Member

@felipeZ felipeZ commented Jun 29, 2020

Changes

Add Gram-Schmidt orthonormalization process

implements #11

@felipeZ felipeZ requested review from JensWehner and NicoRenaud June 29, 2020 11:03
@felipeZ felipeZ marked this pull request as draft June 29, 2020 11:06
@codecov-commenter
Copy link

codecov-commenter commented Jun 29, 2020

Codecov Report

Merging #15 into master will increase coverage by 0.1%.
The diff coverage is 100.0%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master     #15     +/-   ##
========================================
+ Coverage    92.1%   92.3%   +0.1%     
========================================
  Files          39      40      +1     
  Lines        1610    1647     +37     
========================================
+ Hits         1484    1521     +37     
  Misses        126     126             
Impacted Files Coverage Δ
include/Spectra/LinAlg/Orthogonalization.h 100.0% <100.0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 049853e...17b8ea1. Read the comment docs.

@felipeZ felipeZ marked this pull request as ready for review June 29, 2020 12:51
Copy link

@JensWehner JensWehner left a comment

Choose a reason for hiding this comment

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

Do you want to implement the twice is enough here as well?

@felipeZ felipeZ marked this pull request as draft June 29, 2020 13:27
Copy link

@NicoRenaud NicoRenaud left a comment

Choose a reason for hiding this comment

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

class or not class :)

include/Spectra/LinAlg/Orthogonalization.h Outdated Show resolved Hide resolved
@felipeZ felipeZ marked this pull request as ready for review June 29, 2020 14:44
Copy link

@NicoRenaud NicoRenaud left a comment

Choose a reason for hiding this comment

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

few things to change but good :)

include/Spectra/LinAlg/Orthogonalization.h Outdated Show resolved Hide resolved
include/Spectra/LinAlg/Orthogonalization.h Outdated Show resolved Hide resolved
include/Spectra/LinAlg/Orthogonalization.h Outdated Show resolved Hide resolved
Copy link

@JensWehner JensWehner left a comment

Choose a reason for hiding this comment

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

Twice is enough is not implemented yet and Tests for the other orthogonalisation methods are missing.

include/Spectra/LinAlg/Orthogonalization.h Outdated Show resolved Hide resolved
Copy link

@JensWehner JensWehner left a comment

Choose a reason for hiding this comment

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

I like all of it, apart form the check_sanity. I think we should split it into two:

Assert_leftColsToSkipRange()

and maybe treatfirstColumn()

include/Spectra/LinAlg/Orthogonalization.h Outdated Show resolved Hide resolved
@NicoRenaud NicoRenaud requested a review from JensWehner July 2, 2020 11:06
Copy link

@JensWehner JensWehner left a comment

Choose a reason for hiding this comment

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

more small changes

@NicoRenaud NicoRenaud requested a review from JensWehner July 2, 2020 11:33
Copy link

@JensWehner JensWehner left a comment

Choose a reason for hiding this comment

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

Nice work

@JensWehner JensWehner merged commit f7aedcd into master Jul 2, 2020
@JensWehner JensWehner deleted the gramschmidt branch July 2, 2020 11:43
#define SPECTRA_ORTHOGONALIZATION_H

#include <Eigen/Core>
#include <Eigen/Dense>
Copy link
Collaborator

Choose a reason for hiding this comment

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

#include <Eigen/QR> seems sufficient. #include <Eigen/Dense> will also include many heavy solvers such as LU, SVD, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants