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

Implemented upper and lower triangle methods in NumRuby #45

Closed
wants to merge 5 commits into from
Closed

Implemented upper and lower triangle methods in NumRuby #45

wants to merge 5 commits into from

Conversation

iamrajiv
Copy link
Contributor

@iamrajiv iamrajiv commented Jan 20, 2020

Fixes: #29

@iamrajiv iamrajiv requested a review from prasunanand January 21, 2020 09:23
@Uditgulati
Copy link
Member

@iamrajiv Add some tests. Also, indentation needs to be fixed.

m = m.stype(nm_dtype, copy=False)
return m
end

Choose a reason for hiding this comment

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

I think that missing and end

Copy link
Member

Choose a reason for hiding this comment

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

I think lines 72 to 76 are having extra indentation.

def tril(m, k=0)
m = Array(m)
mask = tri(*m.shape[-2:], k=k, nm_dtype=nm_bool)
return where(mask, m, zeros(1, m.nm_dtype))

Choose a reason for hiding this comment

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

I think this return isn't necessary 🤔

def triu(m, k=0)
m = Array(m)
mask = tri(*m.shape[-2:], k=k-1, nm_dtype=nm_bool)
return where(mask, zeros(1, m.nm_dtype), m)

Choose a reason for hiding this comment

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

I think this return isn't necessary 🤔

@Uditgulati
Copy link
Member

@iamrajiv pls have a look at the suggestions.

@@ -62,4 +62,27 @@ def self.hstack(objs)
result
end

def self.tri(N, M=None, k=0, nm_dtype=float)
if M is None:

Choose a reason for hiding this comment

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

I suppose that None is an object that you created. But I think that you can use nil instead of it. Also I think that isn't necessary use the : to the end of sentence. WDYT? 🤔

@Uditgulati
Copy link
Member

@iamrajiv tests are failing, there seems to be some syntax errors. Make sure to run tests first before committing.

@developerfab
Copy link

to resolve the test, I think you could change the Capital Letter in the parameters by lower case, because ruby take the capital letter like a constant. WDYT?

@iamrajiv iamrajiv changed the title added upper and lower triangular matrix Implemented upper and lower triangle methods in NumRuby Dec 8, 2020
@iamrajiv iamrajiv closed this Dec 8, 2020
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.

Implement upper and lower triangle methods in NumRuby
3 participants