Skip to content

[hail] NDArray eye #9105

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

Merged
merged 5 commits into from
Jul 20, 2020
Merged

[hail] NDArray eye #9105

merged 5 commits into from
Jul 20, 2020

Conversation

akotlar
Copy link
Contributor

@akotlar akotlar commented Jul 20, 2020

Identity matrix, needed for ridge regression in Hail. This is obviously suboptimal, it should be a special instance of a sparse ndarray that specifies 'k', but this matches the Numpy implementation and represents a good start.

Copy link
Contributor

@johnc1231 johnc1231 left a comment

Choose a reason for hiding this comment

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

The word "identity" is not present anywhere in the description as far as I can tell. I would include that somewhere for searchability purposes. Or we could just also add the identity function which is basically the same as eye but has a more discoverable name. identity would just be implemented by calling eye. https://numpy.org/doc/stable/reference/generated/numpy.identity.html#numpy.identity

@akotlar
Copy link
Contributor Author

akotlar commented Jul 20, 2020

Good point John. I had omitted "identity" since we didn't have an identity function, but it's worth adding, would help our new docsearch.

If you're back, I can assign you instead.

@akotlar akotlar assigned johnc1231 and unassigned tpoterba Jul 20, 2020
@johnc1231
Copy link
Contributor

I'm back, feel free to assign me to some of these ndarray things. (Also let me know issues you run into when adding stuff)

@akotlar
Copy link
Contributor Author

akotlar commented Jul 20, 2020

Rather than linking out to Numpy, I added an identity function. In a followup PR I will add the "k" argument to eye (I have no need for it now, feel free to PR if you want). Until then. eye differs from identity in that it can generate arbitrary rectangular matrices. Adding "k" will extend that to enable non-main diagonals

Copy link
Contributor

@johnc1231 johnc1231 left a comment

Choose a reason for hiding this comment

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

One change and we'll be good.


Examples
--------
>>> hl.eval(hl.nd.eye(2, dtype=hl.tint32))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should have one example where you pass M, otherwise I think we are good to merge.

Copy link
Contributor

@johnc1231 johnc1231 left a comment

Choose a reason for hiding this comment

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

Thanks!

@danking danking merged commit 59a75e6 into hail-is:master Jul 20, 2020
Dania-Abuhijleh pushed a commit to Dania-Abuhijleh/hail that referenced this pull request Jul 23, 2020
* ndarray eye

* fix

* unnecessary cast

* add identity

* doc M
annamiraotoole pushed a commit to annamiraotoole/hail that referenced this pull request Aug 3, 2020
* ndarray eye

* fix

* unnecessary cast

* add identity

* doc M
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.

4 participants