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 an rrule for the Cholesky decomposition #44

Merged
merged 1 commit into from
Jun 6, 2019
Merged

Conversation

ararslan
Copy link
Member

@ararslan ararslan commented Jun 6, 2019

This is a (mostly) direct port of the code from Nabla.

d = view(A, j, j)
B = view(A, j+1:n, 1:j-1)
c = view(A, j+1:n, j)
end
Copy link
Member

Choose a reason for hiding this comment

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

This mighjt be one of the rare times i suggest wrapping the block in @views

Also didn't we just do out bounds check so we can now @inbounds it.
(again rare time i am going to suggest doing that to a whole l block)

Copy link
Member Author

Choose a reason for hiding this comment

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

The only problem with @views is that AFAICT you aren't guaranteed to actually get a view, since it uses Base.maybeview rather than replacing getindex with view.

Copy link
Member

@oxinabox oxinabox Jun 6, 2019

Choose a reason for hiding this comment

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

Isn't that ok though?
Or does this outright need a view because it is going to mutate it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure, I just wanted to make sure this was the same as Nabla's implementation.

This is a direct port of the code from Nabla.
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.

3 participants