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
Pseudo-inverse of symmetric matrices using SVD / Utility for moving least squares #950
Pseudo-inverse of symmetric matrices using SVD / Utility for moving least squares #950
Changes from 4 commits
a9b9eea
6f67dd3
8acf635
98c2dd8
a4f7326
79580e3
797fd65
d7c4ee5
d3b6e8d
3d105a1
20f906a
0c5bb0d
e2e5838
4568d88
8af21a9
61fff17
8098fc0
2dfac38
e68f83b
4edd575
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.
Is
io.extent(1) == size
a precondition?Presumably
s
andu
must also be properly sized.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.
Yes and yes.
io
,s
andu
must have the same size and this should be guaranteed by the host function. But due to the fact I cannot useassert
in kernels, I skipped checking it. (although, speaking of it, I could useKOKKOS_ASSERT
)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.
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.
Probably a bad idea to name variables
cos
andsin
. If one putsusing namespace std;
somewhere above, this will break.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.
Don't you need to have a tolerance here? What happens if
a = 1
andc = 1+1e-15
?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.
A tolerance is not really needed as the algorithm behaves correctly even if the values are very close together. This case is here to avoid a "true" division by zero.
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.
Dunno if the compiler is smart enough to not do this computation twice.
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.
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.
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.
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.
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.
Probably change all the relevant names
ArborX_Test_DetailsInterpolationSVD
. This would emphasize that it is aDetails
test, as it starts withArborX_Test_Details
. We don't need to emphasize what exact SVD is done, so may omit that. And imho,Interpolation
is better thanInterp
.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 will stick with
InterpDetailsSVD
because it mimics the location and name of the tested file (insrc/interpolation/details
). But maybe would it be better to haveArborX_InterpDetailsSymmetricPseudoInverseSVD.hpp
elsewhere?