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.

Sign up for GitHub

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

Jump to bottom

Sparse omni #834

Merged
merged 10 commits into from Sep 10, 2021
Merged

Sparse omni #834

merged 10 commits into from Sep 10, 2021

Conversation

nicaurvi
Copy link
Contributor

@nicaurvi nicaurvi commented Sep 7, 2021 •
edited

  • Does this PR add any new dependencies?
  • Does this PR modify any existing APIs?
    • Is the change to the API backwards compatible?
  • Have you built the documentation (reference and/or tutorial) and verified the generated documentation is appropriate?

Reference Issues/PRs

What does this implement/fix? Briefly explain your changes.

Any other comments?

daxpryce
daxpryce reviewed Sep 7, 2021
View reviewed changes
graspologic/embed/omni.py Outdated Show resolved Hide resolved
nicaurvi
nicaurvi commented Sep 7, 2021
View reviewed changes
graspologic/embed/omni.py Outdated Show resolved Hide resolved
@nicaurvi
Copy link
Contributor Author

nicaurvi commented Sep 7, 2021

Feel free to add Ben or whoever else is interested from the JHU side; I just added Ali as I believe he did the ASE/LSE sparse PR a while ago.

@nicaurvi nicaurvi self-assigned this Sep 7, 2021
@nicaurvi @asaadeldin11
Update graspologic/utils/utils.py
ad18328 
Co-authored-by: Ali Saad-Eldin <54996865+asaadeldin11@users.noreply.github.com>
pbourke
pbourke reviewed Sep 8, 2021
View reviewed changes
graspologic/embed/omni.py Outdated Show resolved Hide resolved
asaadeldin11
asaadeldin11 reviewed Sep 9, 2021
View reviewed changes
Copy link
Contributor

@asaadeldin11 asaadeldin11 left a comment

Choose a reason for hiding this comment

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

Few minor comments. Also mention in the documentation that omni now accepts sparse inputs.

graspologic/embed/omni.py Outdated Show resolved Hide resolved
graspologic/embed/omni.py Outdated Show resolved Hide resolved
asaadeldin11
asaadeldin11 approved these changes Sep 10, 2021
View reviewed changes
Copy link
Contributor

@asaadeldin11 asaadeldin11 left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks @Nyecarr

@nicaurvi nicaurvi merged commit 2b2a850 into dev Sep 10, 2021
@nicaurvi nicaurvi deleted the sparse-omni branch September 10, 2021 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pbourke pbourke pbourke left review comments

@daxpryce daxpryce daxpryce left review comments

@j1c j1c j1c left review comments

@asaadeldin11 asaadeldin11 asaadeldin11 approved these changes

Assignees

@nicaurvi nicaurvi

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@nicaurvi @pbourke @daxpryce @j1c @asaadeldin11