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.
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
Added accelerated power method to compute eigenpair fast #533
Added accelerated power method to compute eigenpair fast #533
Changes from 16 commits
2afbaaa
e02633c
fc112a4
d6e2546
758c4b0
b7f29a0
fbebd3e
5f50e74
f86ad95
32bf81e
56300ca
cf813c5
4ee4014
f604a97
70cecb3
abfc98e
6d9f95d
844cbea
420272e
afb6ebb
55ad1f1
525dbb6
9c781b6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't agree with having
estimateBeta()
here -- I really don't think it's a good idea to have a heavy-duty operation likeestimateBeta()
be a part of the constructor.Notice, for example, that it would be impossible to construct an instance of this class for later use without that code immediately running. In contrast, if you remove that one function, the constructor becomes basically a no-op.
I think it would be much better to have
beta_
set directly usinginitialBeta
, and leaveestimateBeta()
as a completely separate function that can be called by the user, if needed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: I still think
estimateBeta()
should not be called here ...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jingwuOUO is there a reason
estimateBeta
can't be moved out as a static function??There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should change this description so that the docstring matches the function arguments -- use
x_1
,x_0
, etc.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also added parenthesis in numerator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still see a mismatch between doc and argument names. Also, should be a doxygen comment, /** */
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might consider making the parameter
T
an argument of this function (maybe with the default value 10)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does .rows() work with the factor graph unit tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now using fg.hessian().first (which is also Matrix type), this works.