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

Implement NumRuby::Linalg #30

Merged
merged 4 commits into from Dec 2, 2019
Merged

Implement NumRuby::Linalg #30

merged 4 commits into from Dec 2, 2019

Conversation

Uditgulati
Copy link
Member

No description provided.

*.so
.vscode

Choose a reason for hiding this comment

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

I think you shouldn't add this here, you could place this in a .global-gitignore as you can see here https://gist.github.com/subfuzion/db7f57fff2fb6998a16c WDYT? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

I think *.so should be in .gitignore, .vscode can be shifted to .global-gitignore.

def self.inv(obj)
if obj.is_a?(NMatrix)
return obj.invert
def self.inv(matrix)

Choose a reason for hiding this comment

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

I think you could put this in functions. For example:

def self.inv(matrix)
  is_a_matrix?(matrix)
  valid_shape?(matrix, 2)
  ...
end

private

def is_a_matrix?(matrix)
  raise("Invalid matrix. Not of type NMatrix.") unless matrix.is_a?(NMatrix)
end

def valid_shape?(matrix, shape)
  raise("Invalid shape of matrix. Should be 2.") unless matrix.dim != shape
end

WDYT? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, seems alright. Functions for these would be a good idea as these are quite frequent. I'll add these once the PR is complete. Thanks.

if matrix.dim != 2
raise("Invalid shape of matrix. Should be 2.")
end
if matrix.shape[0] != matrix.shape[1]

Choose a reason for hiding this comment

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

what do you want validate here?

Copy link
Member

Choose a reason for hiding this comment

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

Square matrix, I think

Choose a reason for hiding this comment

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

so I think you could put this in a function as:

def square_matrix?(matrix)
  raise("Invalid shape. Expected square matrix.") unless matrix.shape[0] != matrix.shape[1]
end

WDYT? 🤔

end
m, n = matrix.shape

Choose a reason for hiding this comment

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

what do you main by m and n?

Copy link
Member Author

Choose a reason for hiding this comment

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

m is rows count and n is columns count.

Choose a reason for hiding this comment

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

I think I could name these fields as row and account_ columns to make this more readable. WDYT? 🤔

lu, ipiv = NumRuby::Lapack.getrf(matrix)
inv_a = NumRuby::Lapack.getri(lu, ipiv)

return inv_a

Choose a reason for hiding this comment

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

As this is the last line in this function, it isn't necessary to put return. You could put inv_a and this variable is automatically returned.

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe including return makes it more readable.

@@ -106,9 +165,42 @@ def self.qr(matrix, mode: "full", pivoting: false)
end
m, n = matrix.shape

if pivoting == false
if pivoting == true

Choose a reason for hiding this comment

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

I think you could put this as:

if pivoting
  ...
end

WDYT? 🤔

r = NumRuby.triu(matrix[0...n, 0...n])
end

if pivoting == true

Choose a reason for hiding this comment

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

This validation is similar to line 168. I think you could put this in a function. WDYT? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. cc @Uditgulati

Copy link
Member Author

Choose a reason for hiding this comment

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

A function for an if statement?

Choose a reason for hiding this comment

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

🤔 yes, maybe move only the conditional is weird, but I think that you could the conditional blocks in functions. WDYT? 🤔

@prasunanand prasunanand merged commit ba6ab0c into SciRuby:master Dec 2, 2019
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.

None yet

3 participants